Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20108 )
Change subject: IMPALA-12159: Support ORDER BY for collections of variable length types in select list ...................................................................... Patch Set 5: (4 comments) Still processing descriptors.cc, otherwise lgtm http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/descriptors.cc@937 PS5, Line 937: const NonWritableBasicBlock& insert_before This will be used once sorting collections with varlen slots within structs are supported, right? Can you add a TODO about that? http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/sorter.cc@640 PS5, Line 640: // Recurse first so that children come before parents in the list. Is this an important property? Can you extend the comment to specify whether this is needed or just a random choice? http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/sorter.cc@812 PS5, Line 812: page_start == nullptr Isn't the original version simpler? Due to the DCHECK above the result must be the same. http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/tuple.h File be/src/runtime/tuple.h: http://gerrit.cloudera.org:8080/#/c/20108/5/be/src/runtime/tuple.h@168 PS5, Line 168: /// - the materialized non-NULL string value slots are returned in : /// 'non_null_string_values', : /// - the materialized non-NULL collection value slots, along with their byte sizes, : /// are returned in 'non_null_collection_values' and Can you add a comment about the max size of these? What I mean it that if there are no collection slots, then the max size of the vector can be known from the tuple, but there is no such limit if there are collections with varlen children. -- 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: 5 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: Thu, 09 Nov 2023 14:43:28 +0000 Gerrit-HasComments: Yes
