Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support int4range type #190

Closed
wants to merge 2 commits into from
Closed

feat: support int4range type #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46eacd0
feat: support int4range type
miketheman Oct 9, 2019
1632dff
test: add negative test range
miketheman Oct 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions spec/pg/decoders/range_decoder_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
require "../../spec_helper"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a new spec file to help organize the range specs outside of the regular specs - it looked like that's what happened with array_decoder_spec.cr
I guess if this turns into multiple structs for the different ranges, then maybe the decoder ought to be extracted as well?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don’t have any fully thought out reason, but it kinda makes sense since there will be a lot of related decoders for the ranges.


describe PG::Decoders do
describe "int4range" do
test_decode "exclusive", "'[10,20)'::int4range", Range.new(10, 20)
test_decode "exclusive", "'(10,20]'::int4range", Range.new(11, 21)
test_decode "inclusive", "'[10,20]'::int4range", Range.new(10, 21)
test_decode "inclusive", "'[10,20)'::int4range", Range.new(10, 20)
test_decode "negatives", "'[-14,-5)'::int4range", Range.new(-14, -5)
end
miketheman marked this conversation as resolved.
Show resolved Hide resolved
end
39 changes: 39 additions & 0 deletions src/pg/decoder.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,44 @@ module PG
end
end

struct RangeDecoder
include Decoder

def_oids [
3904, # int4range
]

def decode(io, bytesize, oid)
# General format is here: https://git.io/JeWQX
# Specifics for the binary representation are here: https://git.io/JeWQ1

flags = io.read_byte # first byte is flags. TODO: what do they do?

# TODO: How to use the *_bound_len to inform `read_*` to support int8range
lower_bound_len = read_i32(io) # => 4 or 8
lower_bound = read_i32(io)
upper_bound_len = read_i32(io) # => 4 or 8
upper_bound = read_i32(io)

# some debug cruft here. TODO: remove before merge
# puts "flags: #{flags.to_s}"
# puts "lbound len: #{lower_bound_len}"
# puts "lbound: #{lower_bound}"
# puts "ubound len: #{upper_bound_len}"
# puts "ubound: #{upper_bound}"

# TODO: add `exclusive` flag? Do we need to, or since the decoder
# is already receiving a discrete range type, maybe we don't?
# Ref https://www.postgresql.org/docs/10/rangetypes.html#RANGETYPES-DISCRETE
Range.new(lower_bound, upper_bound)
end

def type
# TODO: do I actually need a new PG-specific type, or can we use real Crystal Ranges?
Range
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a specifically named Int4Range, or should the Range decoder emit a Crystal Range? Is the expectation that the end user desire to be super-specific about whether they are passing an Integer, BigInt, or Numeric type?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I've looked into ranges in crystal, but I think maybe postgres ranges have more "features" than crystal ranges.

In some cases, it is probably easy to normalize a postgres range into a crystal one, like how in the tests you have here the inclusive/exclusive adding 1. However in other cases it might not work, like for the float ranges and date ranges allowing positive and negative infinity. We may have to make our own range class for those. Or enhance the crystal stdlib ranges.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Range can take nil as start and/or end value, so maybe this could be used to represent infinity.

end
end

@@decoders = Hash(Int32, PG::Decoders::Decoder).new(ByteaDecoder.new)

def self.from_oid(oid)
Expand All @@ -497,6 +535,7 @@ module PG
register_decoder Float64Decoder.new
register_decoder TimeDecoder.new
register_decoder NumericDecoder.new
register_decoder RangeDecoder.new
register_decoder PointDecoder.new
register_decoder LineSegmentDecoder.new
register_decoder PathDecoder.new
Expand Down