Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8b17ecb
CSV tests
not-napoleon Nov 27, 2024
7544b5d
allow mixed nanos/millis in type checking
not-napoleon Nov 27, 2024
25646bc
stub methods
not-napoleon Nov 27, 2024
1bc3707
Merge branch 'main' into esql-compare-nanos-and-millis
not-napoleon Nov 27, 2024
b5dae19
Merge branch 'main' into esql-compare-nanos-and-millis
not-napoleon Dec 2, 2024
74b970f
wire up stub evaluators
not-napoleon Dec 2, 2024
edc4e3a
comparator for nanos & millis
not-napoleon Dec 2, 2024
020b83c
wire up the comparator
not-napoleon Dec 2, 2024
9932bc7
tests for not equals
not-napoleon Dec 2, 2024
35f2d4a
Merge branch 'main' into esql-compare-nanos-and-millis
not-napoleon Dec 2, 2024
cae21a3
more tests; not passing yet
not-napoleon Dec 3, 2024
a4a2806
Merge branch 'main' into esql-compare-nanos-and-millis
not-napoleon Dec 4, 2024
add783a
fix comparator, greater than tests
not-napoleon Dec 4, 2024
a7a30be
fix greater than
not-napoleon Dec 4, 2024
2d03fdc
greater than or equal
not-napoleon Dec 4, 2024
1395a80
less than
not-napoleon Dec 4, 2024
292d8b6
less than or equal
not-napoleon Dec 4, 2024
7eef795
spotless apply
not-napoleon Dec 4, 2024
d4dc498
that's been broken for a while
not-napoleon Dec 4, 2024
3bb1417
Update docs/changelog/118027.yaml
not-napoleon Dec 4, 2024
4dbdff9
javadoc
not-napoleon Dec 5, 2024
78e8b9d
Merge branch 'main' into esql-compare-nanos-and-millis
not-napoleon Dec 5, 2024
7048e44
additional tests and comments
not-napoleon Dec 5, 2024
44790fd
Merge remote-tracking branch 'refs/remotes/not-napoleon/esql-compare-…
not-napoleon Dec 5, 2024
af8dab8
spotless apply
not-napoleon Dec 5, 2024
b520da0
assert assumption about nanosecond range
not-napoleon Dec 5, 2024
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
6 changes: 6 additions & 0 deletions docs/changelog/118027.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 118027
summary: Esql compare nanos and millis
area: ES|QL
type: enhancement
issues:
- 116281
36 changes: 36 additions & 0 deletions docs/reference/esql/functions/kibana/definition/equals.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions docs/reference/esql/functions/kibana/definition/greater_than.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions docs/reference/esql/functions/kibana/definition/less_than.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions docs/reference/esql/functions/kibana/definition/not_equals.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions docs/reference/esql/functions/types/equals.asciidoc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions docs/reference/esql/functions/types/greater_than.asciidoc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions docs/reference/esql/functions/types/less_than.asciidoc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions docs/reference/esql/functions/types/not_equals.asciidoc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,37 @@ public static long toMilliSeconds(long nanoSecondsSinceEpoch) {
return nanoSecondsSinceEpoch / 1_000_000;
}

/**
* Compare an epoch nanosecond date (such as returned by {@link DateUtils#toLong}
* to an epoch millisecond date (such as returned by {@link Instant#toEpochMilli()}}.
* <p>
* NB: This function does not implement {@link java.util.Comparator} in
* order to avoid performance costs of autoboxing the input longs.
*
* @param nanos Epoch date represented as a long number of nanoseconds.
* Note that Elasticsearch does not support nanosecond dates
* before Epoch, so this number should never be negative.
* @param millis Epoch date represented as a long number of milliseconds.
* This parameter does not have to be constrained to the
* range of long nanosecond dates.
* @return -1 if the nanosecond date is before the millisecond date,
* 0 if the two dates represent the same instant,
* 1 if the nanosecond date is after the millisecond date
*/
public static int compareNanosToMillis(long nanos, long millis) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the main implementation. All the specific operators are implemented in terms of this method.

Copy link
Member

Choose a reason for hiding this comment

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

Is this comparing sub-second durations, or epoch times? I think it's the former, but please add javadocs to make clear the expectations for input and output.

Copy link
Member Author

Choose a reason for hiding this comment

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

Javadoc added. The intent is in fact to compare epoch dates in the two different formats. The intent here is to support an ES|QL use case comparing fields of different types. This seemed like a good place to put it, since we have a bunch of other functions for dealing with ES date_nanos here, but I can move it if this class isn't appropriate.

assert nanos >= 0;
if (millis < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

If this is sub-second duration (see question above), how can millis be negative? If it's epoch times, why would negative millis the nanos are always greater?

Copy link
Member Author

Choose a reason for hiding this comment

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

Elasticsearch doesn't allow nanosecond resolution dates before epoch, so a millisecond date before epoch will, by definition, be before any valid nanosecond date. See, for example, clampToNanosRange.

Copy link
Member

Choose a reason for hiding this comment

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

We didn't support passing negative epoch millis until recently. Rather than make this assumption, why not make the method correctly compare the two?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do that because we then need to deal with the long overflow case in the comparison. Right now, nanos - (millis * 1_000_000) will never overflow, but if we allow negative values there, then overflow becomes a possibility. We can deal with that, but this seemed more straight forward. I've added some test cases and comments to make this more explicit.

The assumption that nanosecond dates will never be negative is fairly deeply baked into the system, and we would need to change a lot of things before we could support that, not just this method. I could change this to read if (millis < 0 && nanos >= 0) {, even though the second clause will always be true, if you want. I'd rather not add further logic for dealing with currently impossible negative nanos cases though, since none of the other tooling supports it and I'd have to hand-roll conversions (and difficulty in those conversions was why we decided not to support pre-epoch nanos in the first place, IIRC).

Copy link
Member

Choose a reason for hiding this comment

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

Could you add an assert that nanos >= 0? That would then break if we try adding negative support in the future, so modifying this method is not missed.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea. Done.

return 1;
}
if (millis > MAX_NANOSECOND_IN_MILLIS) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it should be a precondition. Why would we have millis outside the expected input bounds?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intent here is to support comparing user data stored in the two different resolutions. So in this case, the millis aren't outside the expected input bounds, as any long-representable datetime should be a valid input. A user might have a query like WHERE nanos_date < millis_date, and millis_date could be past the maximum long-representable nanosecond date, in which case we would still want that expression to evaluate to true. We can optimize that away if the milliseconds are a constant, but if they're in a field, we actually need to compare and get a sensible result.

return -1;
}
// This can't overflow, because we know millis is between 0 and MAX_NANOSECOND_IN_MILLIS,
// and MAX_NANOSECOND_IN_MILLIS * 1_000_000 doesn't overflow.
long diff = nanos - (millis * 1_000_000);
return diff == 0 ? 0 : diff < 0 ? -1 : 1;
}

/**
* Rounds the given utc milliseconds sicne the epoch down to the next unit millis
*
Expand Down
Loading