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 4: (20 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/exprs/variant-functions-ir.cc File be/src/exprs/variant-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/exprs/variant-functions-ir.cc@29 PS3, Line 29: ctx, metadata.ptr, metadata.len, value.ptr, value.len, &result); > We could reserve value.len * [arbitrary number] for less reallocations. Switched to rapidjson and copy the results directly to StringVal. http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/exprs/variant-functions-ir.cc@34 PS3, Line 34: } // namespace impala > StringVal::CopyFrom does the same Done 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 > nit: it's more readable to store it in a separate .h and the implementation For now I'd keep it here as VariantMetadata wouldn't be too useful on its own http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@113 PS3, Line 113: > could be uint32_t Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@124 PS3, Line 124: > could be uint32_t Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@129 PS3, Line 129: IsValid( > nit: IsValid Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@132 PS3, Line 132: uint32_t ReadOffset(uint32_t index) const; : : const uint8_t* offsets_ = nullptr; : const uint8_t* string_data_ = nullptr; : uint32_t dict_size_ = 0; : uint8_t version_ = 0; : uint8_t offset_size_ = 0; // 1, 2, 3, or 4 bytes per offset : bool is_sorted_ = false; : }; : > These fields can be reordered and typed more strictly: Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@152 PS3, Line 152: // - For ARRAY: offset_size_minus_one (bits 2-3), is_large (bit 4) > line too long (96 > 90) Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@158 PS3, Line 158: > nit: int32_t Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@184 PS3, Line 184: (uint32_t index, VariantValue* > nit: it could be a std::string_view Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@202 PS3, Line 202: > metadata is available as a member Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@204 PS3, Line 204: fault of > nit: IsValid, other common accessors could follow the naming pattern Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.h@213 PS3, Line 213: private: > Please add a DCHECK before the memcpy that validates that there's no OOB ac Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.cc File be/src/runtime/variant-value.cc: http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.cc@70 PS3, Line 70: string_data_ = data + pos; : return Status::OK(); : } > This could be unrolled to a switch statement for the 4 possbile cases Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.cc@92 PS3, Line 92: return 0; > StringValue construction could be skipped by reinterpreting the string_data StringValue creation is fairly cheap as smallification is not automatic. Anahow, switched to string_view. http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/runtime/variant-value.cc@115 PS3, Line 115: for (uint32_t i = 0; i < dict_size_; ++i) { > Similarly to L72, it could be unrolled Done 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); > Please add a test for negative cases like empty brackets "$.arr[]", field n Added a few extra tests, and made NavigatePath() stricter as well. LMK if I should add more tests. JSONPath expressions are quite powerful, we only support a narrow subset of them. Later we can add support for more JSONPath exprs. http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/util/variant-util-test.cc@288 PS3, Line 288: // "user":"Alice" > nit: indent Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/util/variant-util-test.cc@459 PS3, Line 459: 0x5f, 0x61, 0x72, 0x72, 0x61, 0x79, 0x75, 0x73, 0x65, 0x72}, > line too long (111 > 90) Done http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/util/variant-util.cc File be/src/util/variant-util.cc: http://gerrit.cloudera.org:8080/#/c/24392/3/be/src/util/variant-util.cc@30 PS3, Line 30: VariantMetadata metadata; > Could we use rapidjson to serialize JSON strings? Switched to rapidjson completely -- 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: Tue, 16 Jun 2026 12:22:45 +0000 Gerrit-HasComments: Yes
