Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16007 )
Change subject: IMPALA-8984: Uncorrelated scalar subqueries in the select list ...................................................................... Patch Set 3: (8 comments) Thanks for the patch. A few comments below..overall it looks pretty good. http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@292 PS3, Line 292: "Invariant violated: Only subqueries that are gauranteed to return a " nit: "guaranteed" http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@937 PS3, Line 937: * supported in FROM and WHERE clauses. The rewrite is performed in place and not in a Update this comment for SELECT clause. http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1117 PS3, Line 1117: * Currently we only support very simple subqueries which return a single aggregate Is this comment applicable for this patch ? Looks like this patch is supporting group-by columns in the subquery with a LIMIT 1 (based on one of your tests). http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1135 PS3, Line 1135: * In this case there is no aggregate function to gaurantee only a single row is nit: spelling 'gaurantee' http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1137 PS3, Line 1137: * would be if the correlated predicates had primary primary key constraints. nit: duplicate 'primary' http://gerrit.cloudera.org:8080/#/c/16007/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1230 PS3, Line 1230: inlineView.setJoinOp(JoinOperator.CROSS_JOIN); If the outer query has 2 tables joined in the FROM clause, then we add this inlineView with a CROSS_JOIN, is there any potential issue that may be uncovered ? I think if the outer tables are inner joined it should be ok. I am not completely sure if there is an outer join ..maybe worth adding a test. http://gerrit.cloudera.org:8080/#/c/16007/3/testdata/workloads/functional-query/queries/QueryTest/subquery.test File testdata/workloads/functional-query/queries/QueryTest/subquery.test: http://gerrit.cloudera.org:8080/#/c/16007/3/testdata/workloads/functional-query/queries/QueryTest/subquery.test@1044 PS3, Line 1044: select id, 1+(select min(id) from functional.alltypessmall) Would be useful to also add the Explain plan for some of these tests either here or as part of PlannerTest. http://gerrit.cloudera.org:8080/#/c/16007/3/testdata/workloads/tpcds/queries/tpcds-decimal_v2-q9.test File testdata/workloads/tpcds/queries/tpcds-decimal_v2-q9.test: http://gerrit.cloudera.org:8080/#/c/16007/3/testdata/workloads/tpcds/queries/tpcds-decimal_v2-q9.test@3 PS3, Line 3: select case when (select count(*) Cool that this query is now supported. Can we also add the plan for this under testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test ? -- To view, visit http://gerrit.cloudera.org:8080/16007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibcf55d26889aa01d69bb85f18c9241dda095fb66 Gerrit-Change-Number: 16007 Gerrit-PatchSet: 3 Gerrit-Owner: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 09 Jun 2020 06:48:43 +0000 Gerrit-HasComments: Yes
