Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
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

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

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
Expand Up @@ -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;
Expand All @@ -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)) {
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 Range should satisfy the contract of expressions, which includes foldability. But we could also just yeet this as we just never reach here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
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.

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'm not really sure what you're suggesting I do about making this more visible

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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())) {
Expand Down