Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19322 )
Change subject: IMPALA-9551: Allow mixed complex types in select list ...................................................................... Patch Set 15: Code-Review+1 (10 comments) looks good to me, just indentation + few more test ideas http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/runtime/complex-value-writer.inline.h File be/src/runtime/complex-value-writer.inline.h: http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/runtime/complex-value-writer.inline.h@90 PS15, Line 90: struct_slot_desc.children_tuple_descriptor(); nit: +2 indentation http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/runtime/complex-value-writer.inline.h@116 PS15, Line 116: reinterpret_cast<CollectionValue*>(child); +2 http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/runtime/complex-value-writer.inline.h@118 PS15, Line 118: child_slot_desc.children_tuple_descriptor(); +2 http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/runtime/complex-value-writer.inline.h@236 PS15, Line 236: children_item_tuple_desc->slots(); +2 http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/service/hs2-util.cc@390 PS15, Line 390: static_cast<const SlotRef&>(scalar_expr).GetSlotDescriptor(); +2 indentation http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/service/query-result-set.cc File be/src/service/query-result-set.cc: http://gerrit.cloudera.org:8080/#/c/19322/15/be/src/service/query-result-set.cc@256 PS15, Line 256: static_cast<const SlotRef&>(scalar_expr).GetSlotDescriptor(); +2 http://gerrit.cloudera.org:8080/#/c/19322/15/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java File fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java: http://gerrit.cloudera.org:8080/#/c/19322/15/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@311 PS15, Line 311: if (colExpr.getType().isStructType()) { else if? http://gerrit.cloudera.org:8080/#/c/19322/15/fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java@417 PS15, Line 417: childSlotDescRawPath.get(childSlotDescRawPath.size() - 1); nit, here and next rows: indentation http://gerrit.cloudera.org:8080/#/c/19322/15/testdata/workloads/functional-query/queries/QueryTest/mixed-collections-and-structs.test File testdata/workloads/functional-query/queries/QueryTest/mixed-collections-and-structs.test: http://gerrit.cloudera.org:8080/#/c/19322/15/testdata/workloads/functional-query/queries/QueryTest/mixed-collections-and-structs.test@257 PS15, Line 257: Zipping unnest If we look at it strictly this is not zipping unnest, just simply unnest, as the zipping part comes in when there are at least 2 columns, e.g arr_contains_nested_struct and arr_contains_struct Other zipping unnest related test idedas: - test zipping unnest in the from clause - test zipping unnest with where filter on an unnested col http://gerrit.cloudera.org:8080/#/c/19322/15/testdata/workloads/functional-query/queries/QueryTest/mixed-collections-and-structs.test@387 PS15, Line 387: WHERE clauses at different levels. Can you also add one where different view levels have where conditions on the same struct member? -- To view, visit http://gerrit.cloudera.org:8080/19322 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I476d98884b5fd192dfcd4feeec7947526aebe993 Gerrit-Change-Number: 19322 Gerrit-PatchSet: 15 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Tue, 31 Jan 2023 15:55:04 +0000 Gerrit-HasComments: Yes
