Qifan Chen 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 4: (10 comments) Looks good! http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG@44 PS4, Line 44: the nit. "a row in a row batch" http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG@58 PS4, Line 58: Restrictions: Since this patch lays the foundation work for STRUCT support (even though the title of the commit is about STRUCT in select list), I wonder if we can formally describe where STRUCTs can appear in DDL and DML. For example 1. In CREATE TABLE DDL; 2. In LOAD statement; 3. IN SELECT statement: a), in SELECT list; b), as a virtual table in JOIN; c), ... ... http://gerrit.cloudera.org:8080/#/c/17638/4//COMMIT_MSG@64 PS4, Line 64: nit. May add items into the test section. http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/service/query-result-set.cc File be/src/service/query-result-set.cc: http://gerrit.cloudera.org:8080/#/c/17638/4/be/src/service/query-result-set.cc@406 PS4, Line 406: DCHECK(col_input.columnType.types.size() > 0); This line probably should be moved to the block for STRUCT. In the ELSE block, we have the assertion for scalar type. http://gerrit.cloudera.org:8080/#/c/17638/4/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/4/testdata/workloads/functional-query/queries/QueryTest/nested-struct-in-select-list.test@56 PS4, Line 56: would be nice to add more cases. 1. select with where clause on the id column; 2. select with where cause on the outer_struct with some SQL string function to qualify a particular field in the struct; 3. select with outer_struct in the select list in a subquery; I also wonder if it is possible to select a particular field from a struct (as the whole thing). http://gerrit.cloudera.org:8080/#/c/17638/4/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/4/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@4 PS4, Line 4: select id, tiny_struct from functional_orc_def.complextypes_structs; nit. Is it possible to say this? If so, I'd like to suggest that we add a couple test queries referencing struct members. Depending on the type of the struct member, we could also apply SQL functions. select id, tiny_struct from functional_orc_def.complextypes_structs my_struct where my_struct.b = false http://gerrit.cloudera.org:8080/#/c/17638/4/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@17 PS4, Line 17: order by id; nit. Can we add one with group by on tiny_struct? Is it supported? http://gerrit.cloudera.org:8080/#/c/17638/4/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@153 PS4, Line 153: and order by a member of the struct nit. Should remove this? I did not see order by clause in the query. http://gerrit.cloudera.org:8080/#/c/17638/4/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@263 PS4, Line 263: joins nit. Can we add other join flavors such as equi join or left join? http://gerrit.cloudera.org:8080/#/c/17638/4/testdata/workloads/functional-query/queries/QueryTest/struct-in-select-list.test@266 PS4, Line 266: tbl, : tbl.nested_struct.c.d as outer_arr, outer_arr.ITEM as inner_arr; nit. Maybe add a comment on the join columns selected from the three tables. -- 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: 4 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Comment-Date: Mon, 02 Aug 2021 18:31:32 +0000 Gerrit-HasComments: Yes
