New output for explain endpoint with Calcite engine #3521
Conversation
Signed-off-by: Lantao Jin <ltjin@amazon.com>
Signed-off-by: Lantao Jin <ltjin@amazon.com>
LantaoJin
changed the title
Signed-off-by: Lantao Jin <ltjin@amazon.com>
LantaoJin
requested review from
GumpacG,
MaxKsyunz,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az and
ykmr1224
as code owners
| if (Strings.isBlank(format)) { | ||
| return ExplainFormat.STANDARD; // Default value | ||
| } |
There was a problem hiding this comment.
Does catch already cover this edge case?
core/src/main/java/org/opensearch/sql/executor/QueryService.java
Outdated
Show resolved
Hide resolved
opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java
Show resolved
Hide resolved
| // format of explain response | ||
| SIMPLE("simple"), | ||
| STANDARD("standard"), | ||
| CODEGEN("codegen"); |
There was a problem hiding this comment.
Explain response format is always JSON?
There was a problem hiding this comment.
Using format is not particularly appropriate, mainly because I want to avoid adding an extra parameter.
_ppl/_explain?format=simple
Do you think should we add a new parameter for it?
There was a problem hiding this comment.
Not sure if I understand this right. In Calcite, explain format is actually referring to JSON, XML, DOT? Ref: https://calcite.apache.org/docs/reference.html. If so, I feel making different explain content as different response format is a little confusing.
There was a problem hiding this comment.
@dai-chen You are refering to Calcite SQL? IMO, it is out of scope of this PR.
@LantaoJin We should consider to add explain command in PPL in future.
| // used in Calcite plan explain | ||
| private final String logical; | ||
| private final String physical; | ||
| private final String codegen; |
There was a problem hiding this comment.
I'm thinking shall we fit all these into existing ExplainResponseNode structure?
There was a problem hiding this comment.
How about add a ExplainResponseNodeV2? The new explain response is plain but ExplainResponseNode is nested.
penghuo
left a comment
penghuo
left a comment
There was a problem hiding this comment.
could u update explain API doc?
integ-test/src/test/resources/expectedOutput/calcite/explain_filter_agg_push.json
Outdated
Show resolved
Hide resolved
integ-test/src/test/resources/expectedOutput/calcite/explain_filter_push.json
Outdated
Show resolved
Hide resolved
protocol/src/main/java/org/opensearch/sql/protocol/response/format/Format.java
Outdated
Show resolved
Hide resolved
updated the endpoint.rst |
Signed-off-by: Lantao Jin <ltjin@amazon.com>
--------- Signed-off-by: Lantao Jin <ltjin@amazon.com> Signed-off-by: xinyual <xinyual@amazon.com>
3 participants
Description
New output format for explain endpoint, see #3519
Related Issues
Resolves #3519
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.