Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16123 )
Change subject: IMPALA-9943,IMPALA-4974: INTERSECT/EXCEPT [DISTINCT] ...................................................................... Patch Set 9: (4 comments) Had a few minor comments but I'm nearly at a +1 http://gerrit.cloudera.org:8080/#/c/16123/9/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/16123/9/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@277 PS9, Line 277: InlineViewRef opWrapperView = null; nit: we could declare the variable on l 309 where it's assigned. http://gerrit.cloudera.org:8080/#/c/16123/9/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@331 PS9, Line 331: eiWrapperView.analyze(analyzer); It doesn't look like we do anything with this view? Is it mean to wrap eiSelect in the set operand? http://gerrit.cloudera.org:8080/#/c/16123/9/testdata/workloads/functional-planner/queries/PlannerTest/setoperation-rewrite.test File testdata/workloads/functional-planner/queries/PlannerTest/setoperation-rewrite.test: http://gerrit.cloudera.org:8080/#/c/16123/9/testdata/workloads/functional-planner/queries/PlannerTest/setoperation-rewrite.test@212 PS9, Line 212: select distinct id, year, month from functional.alltypestiny where year=2009 and month=1 I guess the distinct is sorta serving as an execution hint here, right? Since it would be equivalent with or without the distincts. http://gerrit.cloudera.org:8080/#/c/16123/9/testdata/workloads/functional-query/queries/QueryTest/except.test File testdata/workloads/functional-query/queries/QueryTest/except.test: http://gerrit.cloudera.org:8080/#/c/16123/9/testdata/workloads/functional-query/queries/QueryTest/except.test@616 PS9, Line 616: Can we add an explicit test for except behaviour with nulls, e.g. This should return NULL only - there is no null in alltypes.tinyint_col. select tinyint_col from alltypesagg minus select tinyint_col from alltypes This should return empty because there is a null present. select tinyint_col from alltypesagg minus select tinyint_col from alltypesagg where month = 1 -- To view, visit http://gerrit.cloudera.org:8080/16123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5be46f824217218146ad48b30767af0fc7edbc0f Gerrit-Change-Number: 16123 Gerrit-PatchSet: 9 Gerrit-Owner: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 24 Jul 2020 07:05:45 +0000 Gerrit-HasComments: Yes
