elasticsearch icon indicating copy to clipboard operation
elasticsearch copied to clipboard

[ESQL] Migrate SimplifiyComparisonArithmetics optimization rule

Open not-napoleon opened this issue 1 year ago • 2 comments

This PR migrates the QL optimization rule SimplifyComparisonArithmetics, which folds constant arithmetic in binary comparisons when possible. I didn't find any tests for this optimization rule in the existing code base, so I wrote a few that cover the cases I could think of. I also slightly refactored the optimization to not rely on string comparing the symbols to determine the operation. That was necessary in the generic code, since the operations were in a different module, but by pulling this into the ESQL module, we have direct access to the relevant enum. This commit isolates that refactoring, which might be easier to review.

not-napoleon avatar May 02 '24 14:05 not-napoleon

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine avatar May 02 '24 14:05 elasticsearchmachine

Thanks @astefan , I'll work on getting those tests migrated.

not-napoleon avatar May 03 '24 14:05 not-napoleon

Hi @not-napoleon, I've created a changelog YAML for you.

elasticsearchmachine avatar May 07 '24 20:05 elasticsearchmachine

Hi @not-napoleon, I've updated the changelog YAML for you.

elasticsearchmachine avatar May 07 '24 21:05 elasticsearchmachine

@astefan I finished migrating the tests from OptimizerRunTests (with AwaitsFix notations, as we discussed). I would like to merge this now, without including the SQL spec tests, and add them in a follow up PR. Migrating the spec tests will be a lot of work, as the tests as written assume the existence of a reference database implementation to validate against. Migrating them will require first re-writing them into ESQL, then working out the expected results for them. This PR already contains one real bugfix, and there's several follow up bug fixes that we can't really address until this merges, and I'd rather not delay landing those. What do you think?

not-napoleon avatar May 10 '24 20:05 not-napoleon

After discussion on the general approach to this migration, I am closing this PR in favor of https://github.com/elastic/elasticsearch/pull/108744, which migrates the SQL tests, and the first step of https://github.com/elastic/elasticsearch/issues/106679 which will pull over the relevant optimization rule.

not-napoleon avatar May 16 '24 20:05 not-napoleon