ESQL - CSV tests for invalid ranges #125714
Conversation
not-napoleon
added
>test
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
| */ | ||
| protected boolean areBoundariesInvalid(Object lowerValue, Object upperValue) { | ||
| /* | ||
| NB: I am reasonably sure this code is dead. It can only be reached from foldable(), and as far as I can tell |
There was a problem hiding this comment.
++, I think this is a leftover from ql as we shouldn't represent datetimes as strings inside esql, not even in literals.
There was a problem hiding this comment.
I think the only reason to keep this code at all is because Range should satisfy the contract of expressions, which includes foldability. But we could also just yeet this as we just never reach here.
There was a problem hiding this comment.
the only reason to keep this code at all is because Range should satisfy the contract of expressions, which includes foldability.
We could just throw, like the serialisation method does. But: it'd be great if we decided on supporting range syntax in the language first (issue).
We've had an attempt to bring Range into the logical planning, but eventually settled for the current, more pragmatic approach.
There was a problem hiding this comment.
That issue has been open for a year and a half. As far as I know, it's not planned currently, and it isn't clear that we even want to add that syntax. In the meantime, having to work around dead code adds cognitive load and extra work. If we decide we need that syntax, and that the original logic here is important, we can always find it in the git history.
There was a problem hiding this comment.
having to work around dead code adds cognitive load and extra work
I agree. I was thinking that knowing to look for removed code (and where) is harder than deciding we don't want this syntax and just remove the code already.
There was a problem hiding this comment.
I also think removing this dead code is the right thing to do. I'd be happy to do so, either in this PR or in a follow up, if we're in agreement that we should.
| PushFiltersToSource can also create ranges, but that is a Physical optimizer rule. Folding happens in the | ||
| Logical optimization layer, and should be done by the time we are calling PushFiltersToSource. |
There was a problem hiding this comment.
Thank you for adding this comment, I think the reasoning is solid.
There was a problem hiding this comment.
Thanks a lot @not-napoleon , more tests + very useful comments, I appreciate that a lot!
|
|
||
| // Equals OR Range | ||
| /* | ||
| NB: this loop is probably dead code. There's no syntax for ranges, so the parser never produces them. This |
There was a problem hiding this comment.
Nit, just in case any other changes will be added to the PR:
| NB: this loop is probably dead code. There's no syntax for ranges, so the parser never produces them. This | |
| NB: this loop is currently dead code. There's no syntax for ranges, so the parser never produces them. This |
The "dead code" note in Range.java can be spotted and updated if we bring more Range support in the language, this one might not be as visible.
There was a problem hiding this comment.
I'm not really sure what you're suggesting I do about making this more visible
There was a problem hiding this comment.
My comment was about the possibility of us extending Range support and this comment becoming inaccurate (well, despite the "probably" disclaimer). Not terribly important, can be merged as is.
There was a problem hiding this comment.
I think the three of us think this is dead code. Should we just remove it?
…v-tests' into esql-range-csv-tests
💚 Backport successful
|
Add CSV tests for invalid ranges (i.e. where the lower bound is greater than the upper bound) for several data types. It looks at first glance like there should be a bug here with Date Nanos, but I cannot trigger one, and analysis suggests the code that has the bug is in fact unreachable. I've left comments to that effect.
Add CSV tests for invalid ranges (i.e. where the lower bound is greater than the upper bound) for several data types. It looks at first glance like there should be a bug here with Date Nanos, but I cannot trigger one, and analysis suggests the code that has the bug is in fact unreachable. I've left comments to that effect.
* ESQL - CSV tests for invalid ranges (#125714) Add CSV tests for invalid ranges (i.e. where the lower bound is greater than the upper bound) for several data types. It looks at first glance like there should be a bug here with Date Nanos, but I cannot trigger one, and analysis suggests the code that has the bug is in fact unreachable. I've left comments to that effect. * add capability checks
6 participants
Add CSV tests for invalid ranges (i.e. where the lower bound is greater than the upper bound) for several data types. It looks at first glance like there should be a bug here with Date Nanos, but I cannot trigger one, and analysis suggests the code that has the bug is in fact unreachable. I've left comments to that effect.