[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then enhance the output from the explain command. (1) Show the analyzed sql in the explain header, this is the rewritten sql, which includes implicit casts, and literals are printed with a cast so that their type is visible. (2) When predicates are shown in the plan these are shown in the same format. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The analyzed query text is wrapped at 80 characters. Note that the analyzed query cannot always be executed as queries rewritten to use LEFT SEMI JOIN are not legal sql. In addition some space characters may be removed from the query for prettier display. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=2; EXPLAIN_LEVEL set to 2 [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner Analyzed query: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=4.88MB mem-reservation=0B thread-reservation=2 PLAN-ROOT SINK | mem-estimate=0B mem-reservation=0B thread-reservation=0 | 00:SCAN KUDU [functional_kudu.alltypestiny] predicates: CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) mem-estimate=4.88MB mem-reservation=0B thread-reservation=1 tuple-ids=0 row-size=97B cardinality=1 in pipelines: 00(GETNEXT) Fetched 16 row(s) in 0.03s TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. Add a unit test for the code which wraps text at 60 characters. The output of some Planner Tests in .test files has been updated to include the Analyzed sql that is printed when explain_level is at at least 'extended' level. Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Reviewed-on: http://gerrit.cloudera.org:8080/11719 Tested-by: Impala Public Jenkins Reviewed-by: Thomas Marshall --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 15 Nov 2018 21:32:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 15 Nov 2018 21:31:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 9: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3464/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 15 Nov 2018 17:35:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1367/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 14 Nov 2018 22:04:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then enhance the output from the explain command. (1) Show the analyzed sql in the explain header, this is the rewritten sql, which includes implicit casts, and literals are printed with a cast so that their type is visible. (2) When predicates are shown in the plan these are shown in the same format. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The analyzed query text is wrapped at 80 characters. Note that the analyzed query cannot always be executed as queries rewritten to use LEFT SEMI JOIN are not legal sql. In addition some space characters may be removed from the query for prettier display. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=2; EXPLAIN_LEVEL set to 2 [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner Analyzed query: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=4.88MB mem-reservation=0B thread-reservation=2 PLAN-ROOT SINK | mem-estimate=0B mem-reservation=0B thread-reservation=0 | 00:SCAN KUDU [functional_kudu.alltypestiny] predicates: CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) mem-estimate=4.88MB mem-reservation=0B thread-reservation=1 tuple-ids=0 row-size=97B cardinality=1 in pipelines: 00(GETNEXT) Fetched 16 row(s) in 0.03s TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. Add a unit test for the code which wraps text at 60 characters. The output of some Planner Tests in .test files has been updated to include the Analyzed sql that is printed when explain_level is at at least 'extended' level. Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 07 Nov 2018 03:28:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3435/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:31:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:31:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 06 Nov 2018 23:31:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 6: Code-Review+2 lgtm from my end. will let others on the thread have another look. for maintenance, I'd go with a follow-up to remove ParseNode's toSql(). then for the cases that were default, I'd look if they're really "toSql" or some other type of to-string method. for the non-parseNode types, "toSql" is fine... the differing type guards against the maintenance issue. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 22:27:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1251/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 21:19:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then enhance the output from the explain command. (1) Show the analyzed sql in the explain header, this is the rewritten sql, which includes implicit casts, and literals are printed with a cast so that their type is visible. (2) When predicates are shown in the plan these are shown in the same format. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The analyzed query text is wrapped at 80 characters. Note that the analyzed query cannot always be executed as queries rewritten to use LEFT SEMI JOIN are not legal sql. In addition some space characters may be removed from the query for prettier display. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=2; EXPLAIN_LEVEL set to 2 [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner Analyzed query: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=4.88MB mem-reservation=0B thread-reservation=2 PLAN-ROOT SINK | mem-estimate=0B mem-reservation=0B thread-reservation=0 | 00:SCAN KUDU [functional_kudu.alltypestiny] predicates: CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) mem-estimate=4.88MB mem-reservation=0B thread-reservation=1 tuple-ids=0 row-size=97B cardinality=1 in pipelines: 00(GETNEXT) Fetched 16 row(s) in 0.03s TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. Add a unit test for the code which wraps text at 60 characters. The output of some Planner Tests in .test files has been updated to include the Analyzed sql that is printed when explain_level is at at least 'extended' level. Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: (1 comment) I agree that maintenance is a concern. 1) for ParseNode I could remove toSql() from the interface and force all callers to use toSql(DEFAULT). This is easy in terms of effort as IntelliJ will do it for me, but results in a lot of code changes, especially as many exception messages call toSql(). I did consider doing this originally but wanted to make this change as clear as possible. I can do this quickly in a follow-up jira. There are few more associated classes in analysis which would also be changed. 2) Do you want me to look at changes to toSql() that are not part of the ParseNode interface (or associated classes)? There is org.apache.impala.catalog.Type which has toSql() {return toSql(1)} where the argument controls how deeply we show nested Types. There is org.apache.impala.analysis.TableName.toSql() and org.apache.impala.analysis.PlanHint.toSql() Right now I would propose leaving these as is, but they could also be changed if we wanted to never have a naked toSql() call. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 > ok, just mention it in a brief comment. I prefer such inline constants to h I removed it. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 20:32:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 5: (4 comments) While here, do you have thoughts on how to maintain this going forward? Main concern is that a "toSql()" (default options) will be used and we'll forget to look for casts, if expected. I see that "toSql()" is used in many places other than this use-case so I'd lean towards follow-up changes to require the option and see if we can replace the other "toSql()" calls with something more explicit for their use. I'm fine with a todo/jira. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 > Just some padding for if lines expand. Maybe this is too ugly. ok, just mention it in a brief comment. I prefer such inline constants to have some explanation. It seems to be an optimization so I'm also ok with removing it to simplify. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: col > I don't really understand what is wrong with param? nothing's wrong with it. my comments are motivated to keep things consistent. http://gerrit.cloudera.org:8080/#/c/11719/5/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/5/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@89 PS5, Line 89: * nit: looks like there's still an extra '*' here. http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test File testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test: http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@9 PS4, Line 9: -- +straight_join : * > Yes. Though I welcome your opinion. that's fine by me -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 16:21:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1243/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Thu, 01 Nov 2018 01:18:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then enhance the output from the explain command. (1) Show the analyzed sql in the explain header, this is the rewritten sql, which includes implicit casts, and literals are printed with a cast so that their type is visible. (2) When predicates are shown in the plan these are shown in the same format. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The analyzed query text is wrapped at 80 characters. Note that the analyzed query cannot always be executed as queries rewritten to use LEFT SEMI JOIN are not legal sql. In addition some space characters may be removed from the query for prettier display. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=2; EXPLAIN_LEVEL set to 2 [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner Analyzed query: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=4.88MB mem-reservation=0B thread-reservation=2 PLAN-ROOT SINK | mem-estimate=0B mem-reservation=0B thread-reservation=0 | 00:SCAN KUDU [functional_kudu.alltypestiny] predicates: CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) mem-estimate=4.88MB mem-reservation=0B thread-reservation=1 tuple-ids=0 row-size=97B cardinality=1 in pipelines: 00(GETNEXT) Fetched 16 row(s) in 0.03s TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. Add a unit test for the code which wraps text at 60 characters. The output of some Planner Tests in .test files has been updated to include the Analyzed sql that is printed when explain_level is at at least 'extended' level. Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: (21 comments) Thanks for review comments http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG@51 PS4, Line 51: "" > This quote seems strange here - is it included in the output? This is what you get when you run 'impala-shell.sh -B' http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG@69 PS4, Line 69: 80 > Isn't it 60? yes http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: sql > nit: pls use consistent case (SQL, sql, Sql) Thanks http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: @param > pls omit these javadoc annotations. Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@39 PS4, Line 39:* This should return the same result as calling toSql(ToSqlOptions.DEFAULT). > would be good to do this via a default interface method. pls add a todo for Clever idea http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@25 PS4, Line 25: default, normal > "normal" makes sense if you know what is currently done. perhaps "original Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: // This does have the consequence that the sql with implict casts may not pssibly fail > nit: spelling Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: may not pssibly fail : // to parse if resubmitted as > I found this difficult to understand. Did you mean that Sql produced in thi Yes, rewritten SQL is already not always valid SQL. EXISTS queries that are rewritten as semi-joins are not legal SQL. I clarified the comment slightly http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java File fe/src/main/java/org/apache/impala/analysis/TypeDef.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java@165 PS4, Line 165: return parsedType_.toSql(); > pass down here? It looks like it should, but the parsedType is not a ParseNode so I did not propagate ToSqlOptions to there. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 > what's this? Just some padding for if lines expand. Maybe this is too ugly. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@115 PS4, Line 115: exiting > sp. existing? Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: @pa > remove I don't really understand what is wrong with param? But done. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@451 PS4, Line 451: // AnalyzesOk(stmt.toSql(true), ctx); > rm? Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@750 PS4, Line 750: @para > remove the comment annotations. Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@772 PS4, Line 772: if (line.contains("Analyzed query:")) { : builder.append(line).append("\n"); : inImplictCasts = true; : } else if (inImplictCasts) { : // Keep copying the query with implicit casts. : // This works because this is the last thing in the header. :
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: (15 comments) overall good cleanup. mostly minor comments from my end. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: sql nit: pls use consistent case (SQL, sql, Sql) http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: @param pls omit these javadoc annotations. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@39 PS4, Line 39:* This should return the same result as calling toSql(ToSqlOptions.DEFAULT). would be good to do this via a default interface method. pls add a todo for when we fully move to Java 8. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@25 PS4, Line 25: default, normal "normal" makes sense if you know what is currently done. perhaps "original query without rewrites"? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: // This does have the consequence that the sql with implict casts may not pssibly fail nit: spelling http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: may not pssibly fail : // to parse if resubmitted as I found this difficult to understand. Did you mean that Sql produced in this mode may not be valid Sql? I'd prefer the comment be merged with the comment block on L35. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java File fe/src/main/java/org/apache/impala/analysis/TypeDef.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java@165 PS4, Line 165: return parsedType_.toSql(); pass down here? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 what's this? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@115 PS4, Line 115: exiting sp. existing? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: @pa remove http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@451 PS4, Line 451: // AnalyzesOk(stmt.toSql(true), ctx); rm? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@750 PS4, Line 750: @para remove the comment annotations. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@801 PS4, Line 801: This would also be included if both more direct: Equivalent to enabling INCLUDE_EXPLAIN_HEADER and EXTENDED_EXPLAIN. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@128 PS4, Line 128: @pa rm http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test File testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test: http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@9 PS4, Line 9: -- +straight_join : * were you expecting the hint to print this way? -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType:
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG@51 PS4, Line 51: "" This quote seems strange here - is it included in the output? http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG@69 PS4, Line 69: 80 Isn't it 60? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@772 PS4, Line 772: if (line.contains("Analyzed query:")) { : builder.append(line).append("\n"); : inImplictCasts = true; : } else if (inImplictCasts) { : // Keep copying the query with implicit casts. : // This works because this is the last thing in the header. : builder.append(line).append("\n"); : } optional: this could be expressed in a more compact way: inImplictCasts |= line.contains("Analyzed query:"); if (inImplictCasts) builder.append(line).append("\n"); http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@83 PS4, Line 83: wrap length for testWrapText() - less than 80 to make test layout nicer nit: Capital W + . at the end. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@89 PS4, Line 89: * nit: extra * http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@127 PS4, Line 127:* Check that code that has been wrapped is correctly formatted nit: missing . -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 31 Oct 2018 15:31:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1215/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 30 Oct 2018 21:09:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 4: I added a new patch because rebase was required -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 30 Oct 2018 20:34:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then enhance the output from the explain command. (1) Show the analyzed sql in the explain header, this is the rewritten sql, which includes implicit casts, and literals are printed with a cast so that their type is visible. (2) When predicates are shown in the plan these are shown in the same format. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The analyzed query text is wrapped at 80 characters. Note that the analyzed query cannot always be executed as queries rewritten to use LEFT SEMI JOIN are not legal sql. In addition some space characters may be removed from the query for prettier display. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=2; EXPLAIN_LEVEL set to 2 [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner Analyzed query: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=4.88MB mem-reservation=0B thread-reservation=2 PLAN-ROOT SINK | mem-estimate=0B mem-reservation=0B thread-reservation=0 | 00:SCAN KUDU [functional_kudu.alltypestiny] predicates: CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) mem-estimate=4.88MB mem-reservation=0B thread-reservation=1 tuple-ids=0 row-size=97B cardinality=1 in pipelines: 00(GETNEXT) Fetched 16 row(s) in 0.03s TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. Add a unit test for the code which wraps text at 80 characters. The output of some Planner Tests in .test files has been updated to include the Analyzed sql that is printed when explain_level is at at least 'extended' level. Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1181/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 26 Oct 2018 22:30:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then enhance the output from the explain command. (1) Show the analyzed sql in the explain header, this is the rewritten sql, which includes implicit casts, and literals are printed with a cast so that their type is visible. (2) When predicates are shown in the plan these are shown in the same format. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The analyzed query text is wrapped at 80 characters. Note that the analyzed query cannot always be executed as queries rewritten to use LEFT SEMI JOIN are not legal sql. In addition some space characters may be removed from the query for prettier display. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=2; EXPLAIN_LEVEL set to 2 [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner Analyzed query: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=4.88MB mem-reservation=0B thread-reservation=2 PLAN-ROOT SINK | mem-estimate=0B mem-reservation=0B thread-reservation=0 | 00:SCAN KUDU [functional_kudu.alltypestiny] predicates: CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) mem-estimate=4.88MB mem-reservation=0B thread-reservation=1 tuple-ids=0 row-size=97B cardinality=1 in pipelines: 00(GETNEXT) Fetched 16 row(s) in 0.03s TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. Add a unit test for the code which wraps text at 80 characters. The output of some Planner Tests in .test files has been updated to include the Analyzed sql that is printed when explain_level is at at least 'extended' level. Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 2: (3 comments) > > It seems good to me, but I would ping Greg Rahn in the Jira, > maybe > > he has some ideas. > > I have pinged him > > > I have one issue with the current output: as I saw in the .test > > files, queries are printed in one line by default, which can make > > complex queries very difficult to read. It would be much nicer to > > break them at logical points, but I have no idea how to do it > > easily. > > I agree that a clever printing would be nice, but that sounds > tricky. I could wrap at say the last space before 80 columns with > only small effort, but this will make the header harder to parse in > tests. Should I try this? Doing the simple thing (wrapping at 80) is probably fine. http://gerrit.cloudera.org:8080/#/c/11719/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11719/2//COMMIT_MSG@32 PS2, Line 32: If a query in a .test file produces a diff when run by PlannerTest, : then print the name of the .test file in the output. Could you submit this as a standalone review? If I understand correctly, it doesn't really have anything to do with the rest of the review, and its nice to keep changes relatively focused, esp. since this is already such a large patch. http://gerrit.cloudera.org:8080/#/c/11719/2//COMMIT_MSG@67 PS2, Line 67: Is "Query with implicit casts:" a good description? Any reason not to also show the rewritten sql here and call this something like "Analyzed query" http://gerrit.cloudera.org:8080/#/c/11719/2/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test: http://gerrit.cloudera.org:8080/#/c/11719/2/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@59 PS2, Line 59: c_custkey > 10 not sure how difficult this would be, but it would be nice to include the implicit casts here too -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 19 Oct 2018 22:58:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 2: > > It seems good to me, but I would ping Greg Rahn in the Jira, > maybe > > he has some ideas. > > I have pinged him > > > I have one issue with the current output: as I saw in the .test > > files, queries are printed in one line by default, which can make > > complex queries very difficult to read. It would be much nicer to > > break them at logical points, but I have no idea how to do it > > easily. > > I agree that a clever printing would be nice, but that sounds > tricky. I could wrap at say the last space before 80 columns with > only small effort, but this will make the header harder to parse in > tests. Should I try this? About good places to start new line: the enum could have a property whether to add new lines, and some ParseNodes could add new lines if it is true, e.g before joins and unions. Another idea (of the "if you already touch it, why not rewrite the whole thing" type, so it is optional): if there is an object passed to toSql(), maybe it could be some kind of ToSqlBuilder instead of ToSqlOption. It could contain the implicit cast related information and could be also used as a string builder. The current implementation uses string builders in most nodes, but I think that toSql() can still be O(n^2) in some cases. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 19 Oct 2018 20:22:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 2: > It seems good to me, but I would ping Greg Rahn in the Jira, maybe > he has some ideas. I have pinged him > I have one issue with the current output: as I saw in the .test > files, queries are printed in one line by default, which can make > complex queries very difficult to read. It would be much nicer to > break them at logical points, but I have no idea how to do it > easily. I agree that a clever printing would be nice, but that sounds tricky. I could wrap at say the last space before 80 columns with only small effort, but this will make the header harder to parse in tests. Should I try this? -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 19 Oct 2018 17:04:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 2: > (4 comments) > > Thanks for the review. What do you think about: > Is "Query with implicit casts:" a good description? > Is the EXPLAIN header the right place for this output? It seems good to me, but I would ping Greg Rahn in the Jira, maybe he has some ideas. I have one issue with the current output: as I saw in the .test files, queries are printed in one line by default, which can make complex queries very difficult to read. It would be much nicer to break them at logical points, but I have no idea how to do it easily. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Oct 2018 22:19:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 1: (4 comments) Thanks for the review. What do you think about: Is "Query with implicit casts:" a good description? Is the EXPLAIN header the right place for this output? http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@30 PS1, Line 30: /** :* Returns the SQL string corresponding to this node. :*/ > Can you explain the relation of the two toSql() functions in the comment? I Thanks fro thinking about this. This is a case where default parameters would help. http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@1 PS1, Line 1: package org.apache.impala.analysis; > Apache license is missing (also noticed by the jenkins job). Done http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@344 PS1, Line 344: TestToSqlWithImplicitCasts > Can you add a test (or maybe a few) to check that nested structures like su good idea http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@546 PS1, Line 546: errorLog.append("\nTest file: " + testFile + ".test" : + "\n"); > nit: fits to one line Done -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Oct 2018 22:08:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1092/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Oct 2018 17:28:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 1: (4 comments) Nice change, I really missed this feature! http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@30 PS1, Line 30: /** :* Returns the SQL string corresponding to this node. :*/ Can you explain the relation of the two toSql() functions in the comment? If I understand correctly, then toSql() should always call toSql(ToSqlOptions options) with default option. At the first glance I thought that the parameterless toSql should be removed to avoid possible mistakes, but then I realized the huge number of places where it is called, so now I agree with this solution. http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@1 PS1, Line 1: package org.apache.impala.analysis; Apache license is missing (also noticed by the jenkins job). http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@344 PS1, Line 344: TestToSqlWithImplicitCasts Can you add a test (or maybe a few) to check that nested structures like sub queries and expression trees propagate the option? http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@546 PS1, Line 546: errorLog.append("\nTest file: " + testFile + ".test" : + "\n"); nit: fits to one line -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Oct 2018 16:59:52 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then print the analyzed query as text in the explain header to aid in diagnosis. This query text is generated from the analyzed statement tree, so it includes query rewriting. In addition implicit casts are printed, and numeric literals are printed with a cast so that their type is visible. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If a query in a .test file produces a diff when run by PlannerTest, then print the name of the .test file in the output. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=extended; EXPLAIN_LEVEL set to extended [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Query with implicit casts: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=0B mem-reservation=0B thread-reservation=2 (etc.) Note that the initial "Query:" that is printed originates from impala-shell (suppressed with --quiet), but for neatness I put the new "Query with implicit casts:" output at the top of the header. TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. The output of some Planner Tests in .test files has been updated to include the sql with implict casts that is printed when explain_level is at at least 'extended' level. QUESTIONS FOR REVIEWERS: Is "Query with implicit casts:" a good description? Is the EXPLAIN header the right place for this output? Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 ) Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/1082/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 18 Oct 2018 01:11:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11719 Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. .. IMPALA-5821: Add query with implicit casts to extended explain output. If explain_level is at 'extended' level or higher, then print the analyzed query as text in the explain header to aid in diagnosis. This query text is generated from the analyzed statement tree, so it includes query rewriting. In addition implicit casts are printed, and numeric literals are printed with a cast so that their type is visible. If SHOW_IMPLICIT_CASTS is passed to toSql() then - in CastExpr print the implicit cast - in NumericLiteral print the literal with a cast to show the type Add a PlannerTestOption directive that will force the query text showing implicit casts to be included in the PLAN section of a .test file. The toSql() method can be called on a ParseNode tree to return the sql corresponding ot the tree. In the past toSQl() has been enhanced to print rewritten sql by partially overloading toSql() [with toSql(boolean)]. This current change requires changing toSQl() in many places as NumericLiteral can appear at different points in ia parse tree. To avoid many new fragile overloads of toSql() I added toSql(ToSqlOptions), where ToSqlOptions is an enum which controls the form of the Sql that is returned. This changes many files but is safer and means that any future options to toSql() can be added painlessly. If a query in a .test file produces a diff when run by PlannerTest, then print the name of the .test file in the output. Documentation of this change will be done as IMPALA-7718 EXAMPLE OUTPUT: [localhost:21000] default> set explain_level=extended; EXPLAIN_LEVEL set to extended [localhost:21000] default> explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100; Query: explain select * from functional_kudu.alltypestiny where bigint_col < 1000 / 100 Query with implicit casts: SELECT * FROM functional_kudu.alltypestiny WHERE CAST(bigint_col AS DOUBLE) < CAST(10 AS DOUBLE) Max Per-Host Resource Reservation: Memory=0B Threads=2 Per-Host Resource Estimates: Memory=10MB Codegen disabled by planner "" F00:PLAN FRAGMENT [UNPARTITIONED] hosts=1 instances=1 | Per-Host Resources: mem-estimate=0B mem-reservation=0B thread-reservation=2 (etc.) Note that the initial "Query:" that is printed originates from impala-shell (suppressed with --quiet), but for neatness I put the new "Query with implicit casts:" output at the top of the header. TESTING: All end-to-end tests pass. Added a new test in ExprRewriterTest which prints sql with implict casts for some interesting queries. The output of some Planner Tests in .test files has been updated to include the sql with implict casts that is printed when explain_level is at at least 'extended' level. QUESTIONS FOR REVIEWERS: Is "Query with implicit casts:" a good description? Is the EXPLAIN header the right place for this output? Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 --- M fe/src/main/java/org/apache/impala/analysis/AdminFnStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddDropRangePartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableAddPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterTableDropPartitionStmt.java M fe/src/main/java/org/apache/impala/analysis/AlterViewStmt.java M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java M fe/src/main/java/org/apache/impala/analysis/AnalyticWindow.java M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java M fe/src/main/java/org/apache/impala/analysis/BoolLiteral.java M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java M fe/src/main/java/org/apache/impala/analysis/CastExpr.java M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDataSrcStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java M