[ESQL] Date Trunc function should support date nanos #110008
Assignees
Labels
:Analytics/ES|QL
AKA ESQL
>enhancement
Team:Analytics
Meta label for analytical engine team (ESQL/Aggs/Geo)
team-discuss
Comments
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
not-napoleon
added a commit
that referenced
this issue
Nov 4, 2024
While working on #110008 I discovered that the Date Trunc tests were only running in folding mode, because the interval types are marked as not representable. The correct way to test this is to set the forceLiteral flag for those fields, which will (as the name suggests) force them to be literals even in non-folding tests. Doing that turned up errors in the evaluatorToString tests, which I fixed. There are two big changes here. First, the second parameter to the evaluator is a Rounding instance, not the actual interval. Since Rounding includes some information about the specific rounding in the toString results, I am just using a starts with matcher to validate the majority of the string, rather than trying to reconstruct the expected rounding string. Second, passing in a literal null for the interval parameter folds the whole expression to null, and thus a completely different toString. I added a clause in AnyNullIsNull to account for this. While I was in there, I moved some specific test cases to a different file. I know moving code is something we're trying to minimize right now, but this seemed worth it. The tests in question do not depend on the parameters of the test case, but all methods in the class get run for every set of parameters. This was causing these tests to be run many times with the same values, which bloats our test run time and test count. Moving them to a distinct class means they'll only be executed once per test run. I feel like this benefit outweighs the cost of git history complexity.
not-napoleon
added a commit
to not-napoleon/elasticsearch
that referenced
this issue
Nov 4, 2024
While working on elastic#110008 I discovered that the Date Trunc tests were only running in folding mode, because the interval types are marked as not representable. The correct way to test this is to set the forceLiteral flag for those fields, which will (as the name suggests) force them to be literals even in non-folding tests. Doing that turned up errors in the evaluatorToString tests, which I fixed. There are two big changes here. First, the second parameter to the evaluator is a Rounding instance, not the actual interval. Since Rounding includes some information about the specific rounding in the toString results, I am just using a starts with matcher to validate the majority of the string, rather than trying to reconstruct the expected rounding string. Second, passing in a literal null for the interval parameter folds the whole expression to null, and thus a completely different toString. I added a clause in AnyNullIsNull to account for this. While I was in there, I moved some specific test cases to a different file. I know moving code is something we're trying to minimize right now, but this seemed worth it. The tests in question do not depend on the parameters of the test case, but all methods in the class get run for every set of parameters. This was causing these tests to be run many times with the same values, which bloats our test run time and test count. Moving them to a distinct class means they'll only be executed once per test run. I feel like this benefit outweighs the cost of git history complexity.
elasticsearchmachine
pushed a commit
that referenced
this issue
Nov 4, 2024
While working on #110008 I discovered that the Date Trunc tests were only running in folding mode, because the interval types are marked as not representable. The correct way to test this is to set the forceLiteral flag for those fields, which will (as the name suggests) force them to be literals even in non-folding tests. Doing that turned up errors in the evaluatorToString tests, which I fixed. There are two big changes here. First, the second parameter to the evaluator is a Rounding instance, not the actual interval. Since Rounding includes some information about the specific rounding in the toString results, I am just using a starts with matcher to validate the majority of the string, rather than trying to reconstruct the expected rounding string. Second, passing in a literal null for the interval parameter folds the whole expression to null, and thus a completely different toString. I added a clause in AnyNullIsNull to account for this. While I was in there, I moved some specific test cases to a different file. I know moving code is something we're trying to minimize right now, but this seemed worth it. The tests in question do not depend on the parameters of the test case, but all methods in the class get run for every set of parameters. This was causing these tests to be run many times with the same values, which bloats our test run time and test count. Moving them to a distinct class means they'll only be executed once per test run. I feel like this benefit outweighs the cost of git history complexity.
not-napoleon
added a commit
that referenced
this issue
Nov 7, 2024
Resolves #110008 As discussed elsewhere, this does NOT allow for truncating to a value smaller than a millisecond. Our timespan literal syntax doesn't allow specifying less than a millisecond, and the rounding infrastructure also does not support it. We also had a discussion regarding the return type, and decided that it made sense to keep the type as date_nanos, even though the truncation will always produce a millisecond-rounded (or higher) value. --------- Co-authored-by: Elastic Machine <[email protected]>
not-napoleon
added a commit
to not-napoleon/elasticsearch
that referenced
this issue
Nov 7, 2024
Resolves elastic#110008 As discussed elsewhere, this does NOT allow for truncating to a value smaller than a millisecond. Our timespan literal syntax doesn't allow specifying less than a millisecond, and the rounding infrastructure also does not support it. We also had a discussion regarding the return type, and decided that it made sense to keep the type as date_nanos, even though the truncation will always produce a millisecond-rounded (or higher) value. --------- Co-authored-by: Elastic Machine <[email protected]>
elasticsearchmachine
pushed a commit
that referenced
this issue
Nov 7, 2024
Resolves #110008 As discussed elsewhere, this does NOT allow for truncating to a value smaller than a millisecond. Our timespan literal syntax doesn't allow specifying less than a millisecond, and the rounding infrastructure also does not support it. We also had a discussion regarding the return type, and decided that it made sense to keep the type as date_nanos, even though the truncation will always produce a millisecond-rounded (or higher) value. --------- Co-authored-by: Elastic Machine <[email protected]>
kderusso
pushed a commit
to kderusso/elasticsearch
that referenced
this issue
Nov 7, 2024
Resolves elastic#110008 As discussed elsewhere, this does NOT allow for truncating to a value smaller than a millisecond. Our timespan literal syntax doesn't allow specifying less than a millisecond, and the rounding infrastructure also does not support it. We also had a discussion regarding the return type, and decided that it made sense to keep the type as date_nanos, even though the truncation will always produce a millisecond-rounded (or higher) value. --------- Co-authored-by: Elastic Machine <[email protected]>
jozala
pushed a commit
that referenced
this issue
Nov 13, 2024
While working on #110008 I discovered that the Date Trunc tests were only running in folding mode, because the interval types are marked as not representable. The correct way to test this is to set the forceLiteral flag for those fields, which will (as the name suggests) force them to be literals even in non-folding tests. Doing that turned up errors in the evaluatorToString tests, which I fixed. There are two big changes here. First, the second parameter to the evaluator is a Rounding instance, not the actual interval. Since Rounding includes some information about the specific rounding in the toString results, I am just using a starts with matcher to validate the majority of the string, rather than trying to reconstruct the expected rounding string. Second, passing in a literal null for the interval parameter folds the whole expression to null, and thus a completely different toString. I added a clause in AnyNullIsNull to account for this. While I was in there, I moved some specific test cases to a different file. I know moving code is something we're trying to minimize right now, but this seemed worth it. The tests in question do not depend on the parameters of the test case, but all methods in the class get run for every set of parameters. This was causing these tests to be run many times with the same values, which bloats our test run time and test count. Moving them to a distinct class means they'll only be executed once per test run. I feel like this benefit outweighs the cost of git history complexity.
jozala
pushed a commit
that referenced
this issue
Nov 13, 2024
Resolves #110008 As discussed elsewhere, this does NOT allow for truncating to a value smaller than a millisecond. Our timespan literal syntax doesn't allow specifying less than a millisecond, and the rounding infrastructure also does not support it. We also had a discussion regarding the return type, and decided that it made sense to keep the type as date_nanos, even though the truncation will always produce a millisecond-rounded (or higher) value. --------- Co-authored-by: Elastic Machine <[email protected]>
alexey-ivanov-es
pushed a commit
to alexey-ivanov-es/elasticsearch
that referenced
this issue
Nov 28, 2024
Resolves elastic#110008 As discussed elsewhere, this does NOT allow for truncating to a value smaller than a millisecond. Our timespan literal syntax doesn't allow specifying less than a millisecond, and the rounding infrastructure also does not support it. We also had a discussion regarding the return type, and decided that it made sense to keep the type as date_nanos, even though the truncation will always produce a millisecond-rounded (or higher) value. --------- Co-authored-by: Elastic Machine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
I've labeled this team discuss because it's unclear to me what this support should look like. Do we need to support buckets smaller than one millisecond? Does that have any resource constraints on the number of buckets, or do we need to build some kind of spill-to-disk thing before we do this?
The text was updated successfully, but these errors were encountered: