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

Esql compare nanos and millis #118027

Merged
merged 26 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 20 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,17 @@ public static long toMilliSeconds(long nanoSecondsSinceEpoch) {
return nanoSecondsSinceEpoch / 1_000_000;
}

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.

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;
}
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@
import java.time.ZonedDateTime;
import java.time.temporal.ChronoField;

import static org.elasticsearch.common.time.DateUtils.MAX_MILLIS_BEFORE_MINUS_9999;
import static org.elasticsearch.common.time.DateUtils.MAX_NANOSECOND_INSTANT;
import static org.elasticsearch.common.time.DateUtils.clampToNanosRange;
import static org.elasticsearch.common.time.DateUtils.compareNanosToMillis;
import static org.elasticsearch.common.time.DateUtils.toInstant;
import static org.elasticsearch.common.time.DateUtils.toLong;
import static org.elasticsearch.common.time.DateUtils.toMilliSeconds;
Expand All @@ -31,6 +34,35 @@

public class DateUtilsTests extends ESTestCase {

public void testCompareNanosToMillis() {
assertThat(compareNanosToMillis(toLong(Instant.EPOCH), Instant.EPOCH.toEpochMilli()), is(0));

// millis before epoch
assertCompareInstants(
randomInstantBetween(Instant.EPOCH, MAX_NANOSECOND_INSTANT),
randomInstantBetween(Instant.ofEpochMilli(MAX_MILLIS_BEFORE_MINUS_9999), Instant.ofEpochMilli(-1L))
);

// millis after nanos range
assertCompareInstants(
randomInstantBetween(Instant.EPOCH, MAX_NANOSECOND_INSTANT),
randomInstantBetween(MAX_NANOSECOND_INSTANT.plusMillis(1), Instant.ofEpochMilli(Long.MAX_VALUE))
);

// both in range
Instant nanos = randomInstantBetween(Instant.EPOCH, MAX_NANOSECOND_INSTANT);
Instant millis = randomInstantBetween(Instant.EPOCH, MAX_NANOSECOND_INSTANT);

assertCompareInstants(nanos, millis);
}

/**
* check that compareNanosToMillis is consistent with Instant#compare.
*/
private void assertCompareInstants(Instant nanos, Instant millis) {
assertThat(compareNanosToMillis(toLong(nanos), millis.toEpochMilli()), equalTo(nanos.compareTo(millis)));
}

public void testInstantToLong() {
assertThat(toLong(Instant.EPOCH), is(0L));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ public static Literal randomLiteral(DataType type) {
case UNSIGNED_LONG, LONG, COUNTER_LONG -> randomLong();
case DATE_PERIOD -> Period.of(randomIntBetween(-1000, 1000), randomIntBetween(-13, 13), randomIntBetween(-32, 32));
case DATETIME -> randomMillisUpToYear9999();
case DATE_NANOS -> randomLong();
case DATE_NANOS -> randomLongBetween(0, Long.MAX_VALUE);
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 unrelated, but has been wrong for a while. I changed how we were logging dates in tests, which then triggered an issue with negative values here, thus needing to fix it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, could be simplified to randomNonNegativeLong()

case DOUBLE, SCALED_FLOAT, COUNTER_DOUBLE -> randomDouble();
case FLOAT -> randomFloat();
case HALF_FLOAT -> HalfFloatPoint.sortableShortToHalfFloat(HalfFloatPoint.halfFloatToSortableShort(randomFloat()));
Expand Down
Loading