-
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] Migrate SimplifyComparisonArithmetics optimization #109256
[ESQL] Migrate SimplifyComparisonArithmetics optimization #109256
Conversation
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Hi @not-napoleon, I've created a changelog YAML for you. |
import org.elasticsearch.xpack.esql.core.type.DataType; | ||
import org.elasticsearch.xpack.esql.core.type.DataTypes; | ||
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Neg; | ||
import org.elasticsearch.xpack.esql.expression.predicate.operator.arithmetic.Sub; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the Neg
and Sub
imports to point to the esql version, instead of the esql-core versions, resolves #108743
final Expression apply() { | ||
// force float point folding for FlP field | ||
Literal bcl = operation.dataType().isRational() | ||
? new Literal(bcLiteral.source(), ((Number) bcLiteral.value()).doubleValue(), DataTypes.DOUBLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this to use new Literal
instead of Literal.of
resolves #108388
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…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
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 packageoptimizer.rules
, and put this as a top level class there. I have another PR open that migrates a bunch of other rules intoOptimizerRules
, 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