Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17811 )
Change subject: IMPALA-9498: Allow returning arrays in select list ...................................................................... Patch Set 27: (13 comments) The patch is pretty good! I mainly looked at the tests and just started looking into the FE changes. Left some comments first. http://gerrit.cloudera.org:8080/#/c/17811/27//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17811/27//COMMIT_MSG@15 PS27, Line 15: Returning ARRAYs from inline or HMS views is also supported Does it mean this patch resolve IMPALA-10952? I see one TODO mentioning this JIRA. http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java File fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java: http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java@349 PS27, Line 349: "UNION and EXCEPT are not supported for collection types"); nit: Maybe we should also mention INTERSECT or change this to "Currently only UNION ALL is supported for collection types". http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/main/java/org/apache/impala/authorization/TableMask.java File fe/src/main/java/org/apache/impala/authorization/TableMask.java: http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/main/java/org/apache/impala/authorization/TableMask.java@94 PS27, Line 94: colName.split("\\.").length > 1 nit: can we use colName.contains("\\.")? http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@296 PS27, Line 296: //testCollectionTableRefs("complex_nested_struct_col.f2.f12", "f21"); Does this fail? http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1042 PS27, Line 1042: // TODO (IMPALA-10952): at the moment this query would return an error during : // plan generation, so the analyses is successful It seems working now. Maybe this is a stale comment. http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1045 PS27, Line 1045: "(select int_array_col from functional.allcomplextypes) v"); Can we add another test like this? select pos, item from (select int_array_col from functional.allcomplextypes) v It causes an exception in my env: I1214 15:21:02.198695 4236 jni-util.cc:286] 584376737de25b64:a18cb6fd00000000] java.lang.IllegalStateException at com.google.common.base.Preconditions.checkState(Preconditions.java:492) at org.apache.impala.analysis.SlotRef.toThrift(SlotRef.java:310) at org.apache.impala.analysis.Expr.treeToThriftHelper(Expr.java:865) at org.apache.impala.analysis.Expr.treeToThrift(Expr.java:844) at org.apache.impala.analysis.Expr.treesToThrift(Expr.java:898) at org.apache.impala.planner.PlanRootSink.toThriftImpl(PlanRootSink.java:203) at org.apache.impala.planner.DataSink.toThrift(DataSink.java:93) at org.apache.impala.planner.PlanFragment.toThrift(PlanFragment.java:449) at org.apache.impala.service.Frontend.createPlanExecInfo(Frontend.java:1549) at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1577) at org.apache.impala.service.Frontend.getPlannedExecRequest(Frontend.java:1939) at org.apache.impala.service.Frontend.doCreateExecRequest(Frontend.java:1779) at org.apache.impala.service.Frontend.getTExecRequest(Frontend.java:1644) at org.apache.impala.service.Frontend.createExecRequest(Frontend.java:1614) at org.apache.impala.service.JniFrontend.createExecRequest(JniFrontend.java:164) http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@710 PS27, Line 710: , nit: need one space http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@719 PS27, Line 719: allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT) nit: I think we can simply use TPrivilegeLevel.SELECT here, because it's missing privileges on the other column. http://gerrit.cloudera.org:8080/#/c/17811/27/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java@723 PS27, Line 723: allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT) nit: same as above, I think we can simply use TPrivilegeLevel.SELECT. http://gerrit.cloudera.org:8080/#/c/17811/27/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/17811/27/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test@105 PS27, Line 105: Changing a column to a different type I'm confused that this also get rejected: select 1, int_array from complextypestbl union all select 2, int_array from complextypestbl; But it seems like their column types are the same. http://gerrit.cloudera.org:8080/#/c/17811/27/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test@187 PS27, Line 187: clouse nit: clause http://gerrit.cloudera.org:8080/#/c/17811/27/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test@198 PS27, Line 198: clouse nit: clause http://gerrit.cloudera.org:8080/#/c/17811/27/testdata/workloads/functional-query/queries/QueryTest/zipping-unnest-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/zipping-unnest-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/17811/27/testdata/workloads/functional-query/queries/QueryTest/zipping-unnest-in-select-list.test@170 PS27, Line 170: # IMPALA-9498 Might fix this as it will allow to query arrays in the select list. stale comment? -- To view, visit http://gerrit.cloudera.org:8080/17811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb1e42ffb21c7ddc033aba0f754b0108e46f34d0 Gerrit-Change-Number: 17811 Gerrit-PatchSet: 27 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Tue, 14 Dec 2021 09:20:50 +0000 Gerrit-HasComments: Yes
