Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24392 )
Change subject: IMPALA-15067: Add VariantValue and basic decoding functions ...................................................................... Patch Set 5: (7 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.h File be/src/runtime/variant-value.h: http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.h@200 PS4, Line 200: > nit: Data Done http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.h@201 PS4, Line 201: lid > nit: Len Done http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc File be/src/runtime/variant-value.cc: http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@423 PS4, Line 423: } > nit: using Done http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@457 PS4, Line 457: case VariantPhysicalType::INT32: > nit: could be smaller like 24 Done http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@472 PS4, Line 472: case VariantPhysicalType::STRING: { > It could use DateParser::FormatDefault to a stack buffer to avoid heap allo Good catch, done. http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@500 PS4, Line 500: int scale = val.Data()[1]; > Similarly to DATE, it could use the TZ formatter and a stack buffer instead Done http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@515 PS4, Line 515: TimestampValue ts = TimestampValue::UtcFromUnixTimeMicros(micros); > nit: a Base64Encode with a string_view arg could eliminate the std::string Base64Encode has an overload that takes ptr and len. Now also reserve capacity for result string. -- To view, visit http://gerrit.cloudera.org:8080/24392 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I904618570e8c21d099c9a96b496d85e9246483de Gerrit-Change-Number: 24392 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 18 Jun 2026 09:42:48 +0000 Gerrit-HasComments: Yes
