Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17638 )
Change subject: WiP: IMPALA-9495: Support struct in select list for ORC tables ...................................................................... Patch Set 10: (13 comments) Really excited to see this patch! I will mostly focus on the FE part since the BE part is well reviewed. Left some initial comments. Will do a second round soon. http://gerrit.cloudera.org:8080/#/c/17638/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17638/10//COMMIT_MSG@10 PS10, Line 10: subqueries This confuses me with the changes in Subquery.java. I think only subqueries in the FromClause, i.e. inline views, are supported. Maybe we can remove this. http://gerrit.cloudera.org:8080/#/c/17638/10//COMMIT_MSG@93 PS10, Line 93: inline view. Could you add some simple canary tests about COMPUTE STATS and SHOW COLUMN STATS on these new tables? http://gerrit.cloudera.org:8080/#/c/17638/10/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: http://gerrit.cloudera.org:8080/#/c/17638/10/fe/src/main/java/org/apache/impala/analysis/SortInfo.java@259 PS10, Line 259: if (dstSlotDesc.getType().isStructType() && : dstSlotDesc.getItemTupleDesc() != null) { I may miss some tests but do we have tests on sorting by STRUCT columns? http://gerrit.cloudera.org:8080/#/c/17638/10/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/17638/10/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@610 PS10, Line 610: " m2:map<int,struct<x:int,y:int,m3:map<int,int>>>>>)"); nit: Could you add some tests on selecting struct columns that contain nested collection columns? E.g. select item from d.t7.c3 select item from d.t7.c3.item.a2 select value from d.t7.c5 EDIT: Just saw the new tests below.. We might be just lack of a test for struct containing map columns. http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test@42 PS10, Line 42: ---- RESULTS nit: add VERIFY_IS_EQUAL_SORTED label http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test@56 PS10, Line 56: ---- RESULTS nit: add VERIFY_IS_EQUAL_SORTED label http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@4 PS10, Line 4: select id, tiny_struct from functional_orc_def.complextypes_structs; Could you add a case of SELECT * FROM functional_orc_def.complextypes_structs? And maybe other queries like SELECT *, tiny_struct FROM functional_orc_def.complextypes_structs SELECT *, tiny_struct.* FROM functional_orc_def.complextypes_structs http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@18 PS10, Line 18: ---- RESULTS nit: add VERIFY_IS_EQUAL_SORTED label http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@32 PS10, Line 32: ---- RESULTS nit: add VERIFY_IS_EQUAL_SORTED label http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@61 PS10, Line 61: ---- RESULTS nit: add VERIFY_IS_EQUAL_SORTED label http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@169 PS10, Line 169: ---- RESULTS nit: add VERIFY_IS_EQUAL_SORTED label http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@477 PS10, Line 477: Incompatible return types 'STRUCT<b:BOOLEAN>' and 'STRUCT<b:BOOLEAN>' of exprs 'tiny_struct' and 'tiny_struct'. I feel a bit confused when seeing the two identical types are incompatible.. Can we refine this error message to be sth like "Unioning struct types is not supported"? Or we can ignore this if we are going to support it soon. http://gerrit.cloudera.org:8080/#/c/17638/10/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/17638/10/tests/authorization/test_ranger.py@999 PS10, Line 999: def test_column_masking(self, vector, unique_name): Could you add a test here about masking a STRUCT column to NULL? -- To view, visit http://gerrit.cloudera.org:8080/17638 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fbe56bdcd372b72e99c0195d87a818e7fa4bc3a Gerrit-Change-Number: 17638 Gerrit-PatchSet: 10 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 24 Aug 2021 09:02:15 +0000 Gerrit-HasComments: Yes
