Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/17638 )
Change subject: IMPALA-9495: Support struct in select list for ORC tables ...................................................................... Patch Set 12: (12 comments) Thanks Quanlong for taking a look! 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: When displ > This confuses me with the changes in Subquery.java. I think only subqueries You're right, I'll 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 Done 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? Sorting by struct is not possible. Let me add a test to cover this. You can sort by the primitive members of a struct but that had been supported previously as well. 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 nest Selecting a struct that contains nested collection(s) is not allowed atm. We get an error in this case. See the tests for "AnalysisException: Struct containing a collection type is not allowed in the select list.". I added a test here as well to cover. 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 Done 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 Done 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_struc The reason I haven't written "select *" tests is that they won't exercise the new code. A simple "SELECT *" will ommit the struct columns where "SELECT struct_col.*" is an existing functionality to query the members of the struct. Additionally, there are existing tests for these. One use case I find interesting is to try the second approach with nested structs. I'll add a test for that in nested-struct-in-select-list.test 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 Done 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@169 PS10, Line 169: ---- TYPES > nit: add VERIFY_IS_EQUAL_SORTED label Done http://gerrit.cloudera.org:8080/#/c/17638/10/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@477 PS10, Line 477: types > I feel a bit confused when seeing the two identical types are incompatible. Indeed the error msg is a bit misleading. I found an easy way to print out something more meaningful. -- 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: 12 Gerrit-Owner: Gabor Kaszab <gaborkas...@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: Wed, 25 Aug 2021 15:16:55 +0000 Gerrit-HasComments: Yes
