Guard unresolved canonicalization in aggregate rewrite#145332
Guard unresolved canonicalization in aggregate rewrite#145332MattAlp wants to merge 2 commits intoelastic:mainfrom
Conversation
Avoid canonical() on unresolved expressions in ReplaceAggregateNestedExpressionWithEval so unresolved synthetic step attributes no longer throw UnresolvedException during optimizer dedup. Add rule-level and PromQL regressions covering the failure.
MattAlp
requested review from
alex-spies and
felixbarny
and removed request for
alex-spies
elasticsearchmachine
added
v9.4.0
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @MattAlp, I've created a changelog YAML for you. |
alex-spies
left a comment
alex-spies
left a comment
There was a problem hiding this comment.
Thanks @MattAlp !
I would like to take a different approach, because unresolved expressions shouldn't even pass through the verifier. If we let them through, we have no guarantee anymore that we can call .dataType on expressions further down in the optimization pipeline. (We lose an invariant.) That applies to other method calls that are only valid on resolved expressions as well, and would mean that we need to code extra defensively in the optimizer because PromQL queries may have unresolved expressions.
| private static Expression canonicalIfResolved(Expression expression) { | ||
| return expression.resolved() ? expression.canonical() : expression; | ||
| } |
There was a problem hiding this comment.
Unresolved expressions shouldn't reach here. If they did, the verifier messed up.
This needs to be caught earlier.
There was a problem hiding this comment.
Makes sense, the original issue arises because the PromQL path builds plans differently and sidesteps our invariants + guards (as you mentioned).
There was a problem hiding this comment.
We're also planning to move the promql translation from the optimization phase to the analysis phase. That should also help with cases like this as it would be caught by the post analysis verifier. See also #145384.
There was a problem hiding this comment.
Awesome, thanks @felixbarny ! That seems like the right direction. Let us know if you find friction or this otherwise "doesn't fit in" with the usual flow of ESQL queries.
There was a problem hiding this comment.
Will do. In the meantime, some tactical tightening of the validation:
|
Closing in favor of a different design post-verification-discussions |
4 participants
This bug was caused by
ReplaceAggregateNestedExpressionWithEvalcallingcanonical()on unresolved expressions (for example unresolved synthetic PromQLstep), which ends up callingdataType()and throwingUnresolvedException. The fix is to canonicalize only resolved expressions and use unresolved expressions as-is for dedup keys, so we avoid internal optimizer crashes and continue to return normal user-facing errors. Added tests cover both the rule-level unresolvedstepcase and a PromQL query path that previously hit this failure.This is separate from #145307, I think we want to have a bit of hardening in the optimizer separate from the PromQL-to-ESQL validation steps.