-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[ESQL] SimplifyComparisonArithmetics not being correctly applied #108743
Labels
Comments
elasticsearchmachine
added
the
Team:Analytics
Meta label for analytical engine team (ESQL/Aggs/Geo)
label
May 16, 2024
Pinging @elastic/es-analytical-engine (Team:Analytics) |
not-napoleon
added a commit
that referenced
this issue
Jun 3, 2024
This pulls the SimplifyComparisonArithmetics optimization into ESQL proper, from core, and in doing so resolves two of the issues we uncovered with that optimization. Rather than moving this into a sub-class of OptimzierRules, I've opted to create a new package optimizer.rules, and put this as a top level class there. I have another PR open that migrates a bunch of other rules into OptimizerRules, and that's in general an area of the code that's being touched a lot lately. Having top level classes in a package, rather than sub-classes in a single class file, will create fewer conflicts while working on things that are actually unrelated in this type of situation. Although this is a bugfix, I did not mark it for backporting to 8.14. To my knowledge, no users have reported the associated bugs (we found them through migrating tests), and this fix depends on tests and refactorings that were not backported to 8.14. It would be possible to write an 8.14 version of this, but IMHO it's easier to do as a fresh ticket than as a backport and conflict resolution process. If we think that's important, I'm happy to do so, but at the moment I think fixing it for 8.15 is fine. Note that this is a re-application of the changes in #108200 after we did other refactoring, which that was closed to enable. Resolves #108388 Resolves #108743
craigtaverner
pushed a commit
to craigtaverner/elasticsearch
that referenced
this issue
Jun 11, 2024
…9256) This pulls the SimplifyComparisonArithmetics optimization into ESQL proper, from core, and in doing so resolves two of the issues we uncovered with that optimization. Rather than moving this into a sub-class of OptimzierRules, I've opted to create a new package optimizer.rules, and put this as a top level class there. I have another PR open that migrates a bunch of other rules into OptimizerRules, and that's in general an area of the code that's being touched a lot lately. Having top level classes in a package, rather than sub-classes in a single class file, will create fewer conflicts while working on things that are actually unrelated in this type of situation. Although this is a bugfix, I did not mark it for backporting to 8.14. To my knowledge, no users have reported the associated bugs (we found them through migrating tests), and this fix depends on tests and refactorings that were not backported to 8.14. It would be possible to write an 8.14 version of this, but IMHO it's easier to do as a fresh ticket than as a backport and conflict resolution process. If we think that's important, I'm happy to do so, but at the moment I think fixing it for 8.15 is fine. Note that this is a re-application of the changes in elastic#108200 after we did other refactoring, which that was closed to enable. Resolves elastic#108388 Resolves elastic#108743
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Elasticsearch Version
main
Installed Plugins
No response
Java Version
bundled
OS Version
n/a
Problem Description
The
SimplifyComparisonArithmetics
optimization rule isn't being applied everywhere we expect. We see this as test failures for cases that should be optimized by this rule. There are several related issues, this one is about failures related to using the QLNeg
andSub
classes, instead of the ES|QL versions, in the mentioned optimization.Steps to Reproduce
Run the SQL tests (soon to be ported to ES|QL)
Logs (if relevant)
No response
The text was updated successfully, but these errors were encountered: