Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24635 )
Change subject: IMPALA-15221: Harden VARIANT decoding against corrupt input ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/runtime/variant-value.cc File be/src/runtime/variant-value.cc: http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/runtime/variant-value.cc@89 PS3, Line 89: < can it be == ? I assume that keys can't be 0 bytes http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/runtime/variant-value.cc@123 PS3, Line 123: if (UNLIKELY(index >= dict_size_)) return {}; Can code actually call with larger indexes? My impression is that DCHECK was ok here. http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/runtime/variant-value.cc@235 PS3, Line 235: VariantPhysicalType pt; : if (!AsPrimitive(&pt) || pt != VariantPhysicalType::DOUBLE) Merge to bool checkType(VariantPhysicalType expected)? Or even better, merge to ReadValue by passing it expected type? http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/runtime/variant-value.cc@561 PS3, Line 561: if (ctx) { : ctx->AddWarning("Invalid DATE value in VARIANT"); : } Not sure what is the best way here, but it is awkward that an out of range data is handled as different kind of error than malformed variants. http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/util/variant-util-test.cc File be/src/util/variant-util-test.cc: http://gerrit.cloudera.org:8080/#/c/24635/3/be/src/util/variant-util-test.cc@869 PS3, Line 869: EXPECT_FALSE(m.Init(d.data(), d.size()).ok()); } The syntax loks a bit weird, why not reuse the same "d" bector multiple times? Or use a function like ExpectVariantError -- To view, visit http://gerrit.cloudera.org:8080/24635 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3a22bb16cd05854a9afb56ebd27806ac52670bca Gerrit-Change-Number: 24635 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Sat, 25 Jul 2026 12:44:57 +0000 Gerrit-HasComments: Yes
