Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20108 )
Change subject: WIP: IMPALA-12159: Support ORDER BY for collections of variable length types in select list ...................................................................... Patch Set 3: (9 comments) Understood the BE part 50%, rest of the code 80%. :) Looks great so far! http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/codegen/codegen-anyval-read-write-info.h File be/src/codegen/codegen-anyval-read-write-info.h: http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/codegen/codegen-anyval-read-write-info.h@70 PS3, Line 70: CreateBBlockBefore BBlock means basic block? I think that it would be clearer to keep it in non abbreviated form. Adding a comment would be also nice. http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.h@292 PS3, Line 292: NonWritableBasicBlock As we have discussed offline, it looks unusual to pass NonWritableBasicBlock as value instead of const&, even if it is a very small struct. http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.h@293 PS3, Line 293: Can you mention that the functions below are helper functions to CodegenWriteCollectionChildrenToSlot? http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.cc@1273 PS3, Line 1273: void SlotDescriptor::CodegenWriteCollectionChildrenToSlot(LlvmCodeGen* codegen, : LlvmBuilder* builder, llvm::Value* collection_value_ptr, llvm::Value* num_tuples, : llvm::Value* pool_val, NonWritableBasicBlock insert_before) const { : // We construct a while-like loop using basic blocks and conditional branches to iterate : // through the children of the collection, recursively. naming: "children" makes me associate on the different slots in the item tuple, while we iterated though the items within the collection. I think that CodegenWriteCollectionItemsToSlot would be clearer. http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.cc@1321 PS3, Line 1321: child_index naming: item_index? http://gerrit.cloudera.org:8080/#/c/20108/3/be/src/runtime/descriptors.cc@1331 PS3, Line 1331: llvm::Value* children_tuple_array = builder->CreateBitCast(collection_value_ptr, : children_tuple_type, "children_tuple_array"); : llvm::Value* children_tuple = builder->CreateInBoundsGEP(children_tuple_array, : child_index, "children_tuple"); naming: item_tuple_array, item_tuple? http://gerrit.cloudera.org:8080/#/c/20108/3/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/20108/3/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test@124 PS3, Line 124: # Constants in the select list of unions also lead to a "non-pass-through" union. : select 1, int_array, int_array_array from complextypestbl : union all select 2, int_array, int_array_array from complextypestbl The effect on union all is not mentioned in the commit message. http://gerrit.cloudera.org:8080/#/c/20108/3/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test File testdata/workloads/functional-query/queries/QueryTest/sort-complex.test: http://gerrit.cloudera.org:8080/#/c/20108/3/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test@156 PS3, Line 156: # Sort collections containing structs, also containing var-len data. As we have discussed offline, our coverage for the (not allowed) case when a complex types is used as ordering expression is quite weak. It would be nice to add a few tests for this either here or in FE tests. http://gerrit.cloudera.org:8080/#/c/20108/3/tests/query_test/test_queries.py File tests/query_test/test_queries.py: http://gerrit.cloudera.org:8080/#/c/20108/3/tests/query_test/test_queries.py@141 PS3, Line 141: if file_format == 'hbase': Where do we run top-n-complex now? -- To view, visit http://gerrit.cloudera.org:8080/20108 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic15b29393f260b572e11a8dbb9deeb8c02981852 Gerrit-Change-Number: 20108 Gerrit-PatchSet: 3 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-Comment-Date: Tue, 05 Sep 2023 11:43:23 +0000 Gerrit-HasComments: Yes
