Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/23075 )
Change subject: IMPALA-13892: Add support for printing STRUCTs ...................................................................... Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/runtime/row-batch.h@171 PS6, Line 171: NULL Could use 'nullptr' instead. http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.h@65 PS6, Line 65: second Isn't it the first signature that avoids the copy? http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.h@66 PS6, Line 66: & We usually take output parameters by pointer (applies to the other functions as well). http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc@158 PS6, Line 158: void PrintCollection(const Tuple* t, const SlotDescriptor& slot_d, stringstream& out); If PrintCollection(), PrintStruct() and PrintSlot() are "private" functions, they could be put into an unnamed namespace (or declared 'static'). http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc@171 PS6, Line 171: RawValue::PrintValue(t->GetSlot(slot_d.tuple_offset()), slot_d.type(), -1, &s); Can't we use the overload of PrintValue that takes a stringstream? static void PrintValue(const void* value, const ColumnType& type, int scale, std::stringstream* stream, bool quote_val=false); http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc@181 PS6, Line 181: tuple_d To me, 'item_tuple_d' or 'child_tuple_d' would be easier to understand. http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc@183 PS6, Line 183: Continuation lines should have 4 spaces of indentation. http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc@200 PS6, Line 200: tuple_d To me, 'item_tuple_d' or 'child_tuple_d' would be easier to understand. http://gerrit.cloudera.org:8080/#/c/23075/6/be/src/util/debug-util.cc@204 PS6, Line 204: slot_d.type() This could be 'struct_type'. http://gerrit.cloudera.org:8080/#/c/23075/6/tests/custom_cluster/test_tuple_cache.py File tests/custom_cluster/test_tuple_cache.py: http://gerrit.cloudera.org:8080/#/c/23075/6/tests/custom_cluster/test_tuple_cache.py@446 PS6, Line 446: verification_on I think 'exec_option' or 'query_options' describes this variable better. http://gerrit.cloudera.org:8080/#/c/23075/6/tests/custom_cluster/test_tuple_cache.py@451 PS6, Line 451: order by id A limitation of complex types is that sorting is not supported when the tuple includes a collection nested in a struct, and "functional_parquet.complextypestbl.nested_struct" is such a column. See IMPALA-12160. We could sort the result set in Python instead. http://gerrit.cloudera.org:8080/#/c/23075/6/tests/custom_cluster/test_tuple_cache.py@451 PS6, Line 451: * By default, star expressions do not include complex types for backward compatibility reasons. We should either list all columns explicitly or set the EXPAND_COMPLEX_TYPES query option. -- To view, visit http://gerrit.cloudera.org:8080/23075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9479754c2766a9dd6483ba065e26a4d3a22e7e9 Gerrit-Change-Number: 23075 Gerrit-PatchSet: 6 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 01 Jul 2025 12:18:42 +0000 Gerrit-HasComments: Yes
