-
Notifications
You must be signed in to change notification settings - Fork 2
Add sort_ranges parameter and range_combine_end method for PostgreSQL 14 compatibility #28
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.
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
base: master
Are you sure you want to change the base?
Conversation
…finding overlaps.
|
Hi Danieth, Thank you for submitting your PR. I really appreciate your contribution and I'm looking forward to taking a closer look later. However, before that, could you please fix the indentation issue? Once that's done, I'll be able to review it more effectively. Thank you again for your hard work! |
|
Yes, it's fixed now! |
|
@khiav223577 bumping this for review. I've realized, it may make sense to limit the scope of this solution to be less Postgres specific - an alternative solution might look like: It would be a lot simpler. Just a thought. |
* My fork adds support for Time & ActiveSupport::TimeWithZone. Without direct support for these fields, the MultiRange library is very limited. * My fork also adds support for skipping sorting, which, should lead to a tremendous speedup when reading the already sorted ranges from the database. PR on MultiRange gem: khiav223577/multi_range#28
| require 'active_support/time_with_zone' | ||
| require 'active_support/time' | ||
| Time.zone = 'Eastern Time (US & Canada)' | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case fails at Line 131. It may need to add require "active_support/isolated_execution_state" here?
The error message is:
ContainOverlapTest#test_active_support_time_with_timezone_overlaps:
NameError: uninitialized constant ActiveSupport::IsolatedExecutionState
/home/runner/work/multi_range/multi_range/vendor/bundle/ruby/2.7.0/gems/activesupport-7.0.4.3/lib/active_support/core_ext/time/zones.rb:42:in `zone='
/home/runner/work/multi_range/multi_range/test/contain_overlap_test.rb:131:in `test_active_support_time_with_timezone_overlaps'
| @is_float = @ranges.any?{|range| range.begin.is_a?(Float) || range.end.is_a?(Float) } | ||
| @ranges = ranges.map{ |s| s.is_a?(Numeric) ? s..s : s } | ||
| @ranges = @ranges.sort_by(&:begin) if sort_ranges | ||
| @ranges = @ranges.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MINOR: We don't have to reassign the value here.
| @ranges = @ranges.freeze | |
| @ranges.freeze |
|
Others LGTM! |
range_combine_endmethodPostgreSQL 14 handles overlaps slightly differently than the current implementation.
IntegerIntegerFloatDateTimeActiveSupport::TimeWithZoneThe existing logic to handle overlaps works perfectly for Integers, Floats & Dates. This logic, however, does not work for Time or ActiveSupport::TimeWithZone.
The current logic (
+ 1) adds 1 day toTime,ActiveSupport::TimeWithZone- when merge overlaps is called, this can lead to ranges being merged that wouldn't be merged in PostgreSQL.PostgreSQL truncates timestamps to microseconds and then merges ranges that overlap - I duplicated this logic by adding nanoseconds to handle rounding the end temporarily and then checking if the end is greater or equal to the beginning of the next range.
PostgreSQL is the only Database to have Multiranges - so I believe it is ok for this library duplicate the logic present there.
sort_rangesparameter to initializerWhen working with an already sorted multirange (like when pulling PostgreSQL Multiranges directly from the database), it's going to be a lot faster to have the MultiRange skip sorting the ranges.
I believe this parameter could also be used internally to make the data structure faster in some cases (When creating new ranges during
difference&intersectionfor example).closes #27