Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/23075 )
Change subject: IMPALA-13892: Add support for printing STRUCTs ...................................................................... Patch Set 6: (11 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. There were only two usages of NULL, so I switched both to nullptr. 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? Changed this to refer to the "stringstream signature" to avoid confusion. 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 function Switched to a pointer 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 Changed them to be static functions 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? Switched to the overload that takes the stringstream. This required adding support for TYPE_FIXED_UDA_INTERMEDIATE. Since my use case cares about the actual value, I now treat TYPE_FIXED_UDA_INTERMEDIATE like a binary blob. 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. Switched to child_tuple_d 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. Done 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. Switched to child_tuple_d 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'. Done 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. When there are a limited set of query options that do one clear thing, I prefer a name that describes what the options are doing vs something generic. Maybe verification_on_options would be better. Since this now needs expand_complex_types=true, it's harder to have a clear descriptive name, so I changed this to custom_options. 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 comp Good point, added expand_complex_types=true and changed to sorting in Python. -- 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: Mon, 07 Jul 2025 23:23:54 +0000 Gerrit-HasComments: Yes
