Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/24392 )
Change subject: IMPALA-15067: Add VariantValue and basic decoding functions ...................................................................... Patch Set 4: Code-Review+1 (10 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: data nit: Data http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.h@201 PS4, Line 201: len nit: Len http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h File be/src/runtime/variant-value.h: http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@108 PS3, Line 108: Dictionary siz > For now I'd keep it here as VariantMetadata wouldn't be too useful on its o Ack 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: typedef rapidjson::Writer<rapidjson::StringBuffer> JsonWriter; nit: using http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@457 PS4, Line 457: char buf[64]; nit: could be smaller like 24 http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@472 PS4, Line 472: string s = dv.ToString(); It could use DateParser::FormatDefault to a stack buffer to avoid heap allocation for string http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@500 PS4, Line 500: string s = ts.ToString(); Similarly to DATE, it could use the TZ formatter and a stack buffer instead http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/runtime/variant-value.cc@515 PS4, Line 515: Base64Encode(string(sv.Ptr(), sv.Len()), &encoded); nit: a Base64Encode with a string_view arg could eliminate the std::string ctor call http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/util/variant-util-test.cc File be/src/util/variant-util-test.cc: http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/util/variant-util-test.cc@260 PS3, Line 260: vector<uint8_t> meta_bytes = BuildMetadata(names); > Added a few extra tests, and made NavigatePath() stricter as well. LMK if I Ack http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/util/variant-util.h File be/src/util/variant-util.h: http://gerrit.cloudera.org:8080/#/c/24392/4/be/src/util/variant-util.h@20 PS4, Line 20: #include "runtime/variant-value.h" nit: unused -- 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: 4 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: Wed, 17 Jun 2026 14:45:51 +0000 Gerrit-HasComments: Yes
