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.

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

issue #2528 - change interpretation of lt and gt for date searches #2531

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

lmsurpre
Copy link
Member

@lmsurpre lmsurpre commented Jun 18, 2021

We use the range interpretation of the search prefixes for both date and
number/quantity searches. The spec says:

  • lt means "the range below the search value intersects (i.e. overlaps) with the
    range of the target value"
  • gt means "the range above the search value intersects (i.e. overlaps) with the
    range of the target value"

For numbers, the spec additionally says:

  • When a comparison prefix in the set gt, lt, ge, le, sa & eb is provided, the implicit precision of the number is ignored, and they are treated as if they have arbitrarily high precision

Previously, we interpreted date search in the same way and so a search like gt2016
would return any value above 2016-01-01 00:00:00 in the server's
timezone. However, we were not consistent with this because, for some reason,
we decided that lt2016 should return any value below 2017-01-01 00:00:00.

However, even if we properly implemented this "arbitrarily high precision" clause for date search,
this interpretation leads to confusing and unintuitive results like gt2016 returning dates in 2016.
Instead, we will now interpret the "search value" of 2016 to be the
range [2016-01-01 00:00:00, 2017-01-01 00:00:00) and therefor a search for gt2016 will
only find target values above 2017-01-01 00:00:00 (inclusive).

Signed-off-by: Lee Surprenant lmsurpre@us.ibm.com

@lmsurpre lmsurpre force-pushed the issue-2528 branch 3 times, most recently from 9d015f3 to 0958f20 Compare June 18, 2021 17:58
@prb112
Copy link
Contributor

prb112 commented Jun 18, 2021

Should the new builder util be updated with this change?

@lmsurpre lmsurpre force-pushed the issue-2528 branch 3 times, most recently from 9d1d0bb to ce7f76d Compare June 19, 2021 01:44
For both date and number searches.

We use the range interpretation of the search prefixes. The spec says
`lt` means "the range below the search value intersects (i.e. overlaps)
with the range of the target value" and `gt` means "the range above the
search value intersects (i.e. overlaps) with the range of the target
value"

Previously, we interpreted that to mean that a search like `gt2016`
should return any value above `2016-01-01 00:00:00` in the server's
timezone.

However, this interpretation leads to confusing and unintuitive results.
Instead, we will now interpret the "search value" of 2016 to be the
range `[2016-01-01 00:00:00, 2017-01-01 00:00:00)` and therefor it will
only find target values above 2017-01-01 00:00:00 (inclusive).

And similarly for `lt`.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
Copy link
Collaborator

@JohnTimm JohnTimm left a comment

Choose a reason for hiding this comment

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

LGTM

@lmsurpre
Copy link
Member Author

Should the new builder util be updated with this change?

I reworked the changes so that its now contained in the persistence layer and I updated both the old and new behavior classes

@lmsurpre lmsurpre merged commit 05aba06 into main Jun 21, 2021
@lmsurpre lmsurpre deleted the issue-2528 branch June 21, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants