-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL - CSV tests for invalid ranges #125714
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
Changes from all commits
95f38f1
f8027f1
a877554
db023f2
3b74cec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| Invalid boundaries integer literal | ||
|
|
||
| FROM employees | ||
| | WHERE salary > 200 AND salary < 100 | ||
| | KEEP salary; | ||
|
|
||
| salary:integer | ||
| ; | ||
|
|
||
| Invalid boundaries date | ||
|
|
||
| FROM employees | ||
| | WHERE hire_date > TO_DATETIME("2025-01-01") AND hire_date < TO_DATETIME("2020-01-01") | ||
| | KEEP hire_date; | ||
|
|
||
| hire_date:datetime | ||
| ; | ||
|
|
||
| Invalid boundaries, date with implicit casting | ||
|
|
||
| FROM employees | ||
| | WHERE hire_date > "2025-01-01" AND hire_date < "2020-01-01" | ||
| | KEEP hire_date; | ||
|
|
||
| hire_date:datetime | ||
| ; | ||
|
|
||
| Invalid Boundaries, date nanos | ||
| required_capability: date_nanos_type | ||
| required_capability: to_date_nanos | ||
| required_capability: date_nanos_binary_comparison | ||
|
|
||
| FROM date_nanos | ||
| | WHERE nanos > TO_DATE_NANOS("2025-01-01") and nanos < TO_DATE_NANOS("2020-01-01") | ||
| | KEEP nanos; | ||
| warningRegex:Line 2:49: evaluation of \[nanos < TO_DATE_NANOS\(\\\"2020-01-01\\\"\)\] failed, treating result as null\. Only first 20 failures recorded\. | ||
| warningRegex:Line 2:49: java\.lang\.IllegalArgumentException: single-value function encountered multi-value | ||
| warningRegex:Line 2:9: evaluation of \[nanos > TO_DATE_NANOS\(\\\"2025-01-01\\\"\)\] failed, treating result as null\. Only first 20 failures recorded\. | ||
| warningRegex:Line 2:9: java\.lang\.IllegalArgumentException: single-value function encountered multi-value | ||
|
|
||
| nanos:date_nanos | ||
| ; | ||
|
|
||
| Invalid Boundaries, date nanos implicit casting | ||
| required_capability: date_nanos_type | ||
| required_capability: date_nanos_binary_comparison | ||
| required_capability: date_nanos_implicit_casting | ||
|
|
||
| FROM date_nanos | ||
| | WHERE nanos > "2025-01-01" and nanos < "2020-01-01" | ||
| | KEEP nanos; | ||
| warningRegex:Line 2:34: evaluation of \[nanos < \\\"2020-01-01\\\"\] failed, treating result as null\. Only first 20 failures recorded\. | ||
| warningRegex:Line 2:34: java\.lang\.IllegalArgumentException: single-value function encountered multi-value | ||
| warningRegex:Line 2:9: evaluation of \[nanos > \\\"2025-01-01\\\"\] failed, treating result as null\. Only first 20 failures recorded\. | ||
| warningRegex:Line 2:9: java\.lang\.IllegalArgumentException: single-value function encountered multi-value | ||
|
|
||
| nanos:date_nanos | ||
| ; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,6 +115,7 @@ public ZoneId zoneId() { | |
| */ | ||
| @Override | ||
| public boolean foldable() { | ||
| // NB: this is likely dead code. See note in areBoundariesInvalid | ||
| if (lower.foldable() && upper.foldable()) { | ||
| if (value().foldable()) { | ||
| return true; | ||
|
|
@@ -130,6 +131,7 @@ public boolean foldable() { | |
|
|
||
| @Override | ||
| public Object fold(FoldContext ctx) { | ||
| // NB: this is likely dead code. See note in areBoundariesInvalid | ||
| Object lowerValue = lower.fold(ctx); | ||
| Object upperValue = upper.fold(ctx); | ||
| if (areBoundariesInvalid(lowerValue, upperValue)) { | ||
|
|
@@ -149,6 +151,19 @@ public Object fold(FoldContext ctx) { | |
| * If they are, the value does not have to be evaluated. | ||
| */ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++, 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the only reason to keep this code at all is because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| we never fold ranges. There's no ES|QL syntax for ranges, so they can never be created by the parser. The | ||
| PropagateEquals optimizer rule can in theory create ranges, but only from existing ranges. The fact that this | ||
| class is not serializable (note that writeTo throws UnsupportedOperationException) is a clear indicator that | ||
| logical planning cannot output Range nodes. | ||
| 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. | ||
|
Comment on lines
+161
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for adding this comment, I think the reasoning is solid. |
||
| That said, if somehow you have arrived here while debugging something, know that this method is likely | ||
| completely broken for date_nanos, and possibly other types. | ||
| */ | ||
| if (DataType.isDateTime(value.dataType()) || DataType.isDateTime(lower.dataType()) || DataType.isDateTime(upper.dataType())) { | ||
| try { | ||
| if (upperValue instanceof String upperString) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -256,6 +256,11 @@ private static Expression propagate(Or or, LogicalOptimizerContext ctx) { | |||||
| } | ||||||
|
|
||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, just in case any other changes will be added to the PR:
Suggested change
The "dead code" note in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not really sure what you're suggesting I do about making this more visible There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the three of us think this is dead code. Should we just remove it? |
||||||
| rule can create ranges, but only in this loop, which iterates over the existing ranges. In short, | ||||||
| ranges.size() should always be zero at this point. | ||||||
| */ | ||||||
| for (int i = 0; i < ranges.size(); i++) { // might modify list, so use index loop | ||||||
| Range range = ranges.get(i); | ||||||
| if (eq.left().semanticEquals(range.value())) { | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.