-
Notifications
You must be signed in to change notification settings - Fork 25.2k
SQL: Enchance output of explain for optimized plan #33718
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
Conversation
This job triggered CI during a migration of the master. Kicking off an additional build for you manually... Jenkins, test this please. |
1 similar comment
This job triggered CI during a migration of the master. Kicking off an additional build for you manually... Jenkins, test this please. |
Pinging @elastic/es-search-aggs |
@elasticmachine retest this please |
1 similar comment
@elasticmachine retest this please |
assertThat(command("EXPLAIN (PLAN OPTIMIZED) SELECT POWER(i, (1 - 2)) * ABS(10 + 20) FROM test"), | ||
containsString("plan")); | ||
assertThat(readLine(), startsWith("----------")); | ||
assertThat(readLine(), startsWith("Project[[((POWER(i,(1 - 2))) * ABS((10 + 20)))#")); |
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.
Shouldn't this plan have the constants (1-2 and ABS(10+20)) folded?
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.
I have updated the code but force pushed ... (sorry :-( ).
Added this: https://github.com/elastic/elasticsearch/pull/33718/files#diff-0d1503677f70eae9fa10767e44075cecR94 so the inner functions are also folded: https://github.com/elastic/elasticsearch/pull/33718/files#diff-694b8d3dd996b57e56c623dfa0a796fdR170
https://github.com/elastic/elasticsearch/pull/33718/files#diff-694b8d3dd996b57e56c623dfa0a796fdR177
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
Added 6.5 as label |
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
When a literal expression is replaced with its evaluated value, in the optimizer, then use this new evaluated value in the explain plan instead of the original expression.
8caca4e
to
66fccef
Compare
Can we conclude this PR? is it still needed or was it solved already in the meantime? |
@costin I think it's ready now. I added this: 66fccef#diff-0d1503677f70eae9fa10767e44075cecR96 (Unfortunately I messed up something and I forced pushed). The literal expressions are now folded: 66fccef#diff-694b8d3dd996b57e56c623dfa0a796fdR177 |
StringJoiner sj = new StringJoiner(",", "(", ")"); | ||
for (Expression child : children()) { | ||
String val; | ||
if (child instanceof Function) { |
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.
This looks like a good case of overloading/method dispatch.
Literal already returns its value or, in case it is named, name=value.
Function does something similar I believe (especially for operators).
Closing as obsolete. |
When a literal expression is replaced with its evaluated value,
in the optimizer, then use this new evaluated value in the explain plan
instead of the original expression.