ESQL: Do not fold in Range.foldable #119766
Conversation
The upper and lower bounds can be incompatible, in which case the range is foldable to FALSE independently of the foldability of the field. But we shouldn't try and fold the bounds to find out. Calling `foldable` really shouldn't already perform folding. Instead, only perform the check for the invalid bounds if the bounds are already literals. This means that folding will still take place, but it requires the range's child expressions to be folded first. That being said, Range is not used until the physical optimizer stage, anyway.
alex-spies
added
auto-backport
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
|
Hi @alex-spies, I've created a changelog YAML for you. |
| /** | ||
| * In case that the boundaries are not yet folded, but turn out to be invalid after folding, this will still return {@code false}. | ||
| * That's because we shouldn't perform folding when trying to determine foldability. | ||
| */ | ||
| @Override | ||
| public boolean foldable() { |
There was a problem hiding this comment.
If ES|QL coordinator/logicalplanner will not see Range, it is only visible at data node, I wonder if it is a good idea to just return false or an exception if the foldable() or fold() on Range is called?
There was a problem hiding this comment.
I was contemplating just ripping out Range-related code that is currently dead. There's also code in the PropagateEquals optimizer rule that uses Ranges that we know is actually dead because Ranges are only created in the physical optimizer stage.
I think returning false here would be wrong (as we'd break the Expression contract); throwing UnsupportedOperationException would be okay in theory, but I'm hesitant about it since we could still be making calls to foldable/fold in the physical optimizer stage, where ranges are being used. Maybe we don't! But determining this requires careful analysis.
For this reason, I'm leaning towards implementing Range's methods correctly, even though they may be unused.
What do you think?
The upper and lower bounds can be incompatible, in which case the range is foldable to FALSE independently of the foldability of the field.
But we shouldn't try and fold the bounds to find out. Calling
foldablereally shouldn't already perform folding.Instead, only perform the check for the invalid bounds if the bounds are already literals. This means that folding will still take place, but it requires the range's child expressions to be folded first.
That being said, Range is not used until the physical optimizer stage, anyway.