Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/24557 )
Change subject: IMPALA-15057: Add variant_get() builtin and first-class VARIANT expressions ...................................................................... Patch Set 5: (7 comments) Looks good, part1 of comments http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/anyval-util.h File be/src/exprs/anyval-util.h: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/anyval-util.h@363 PS5, Line 363: static void SetAnyValFromEvalResult( I don't see the added value by this function http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr-evaluator.cc File be/src/exprs/scalar-expr-evaluator.cc: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr-evaluator.cc@99 PS5, Line 99: // STRUCT SlotRefs assemble a StructVal from per-child evaluators. VARIANT does not: : // a VARIANT value is read directly into a self-contained VariantVal, and a : // VARIANT-returning function (e.g. variant_get()) evaluates its arguments through the : // normal scalar-function path, not child evaluators. This comment might be better placed somewhere near the VariantVal http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr-evaluator.cc@386 PS5, Line 386: (two StringVals: metadata : // and value) nit: not needed to mention http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr-evaluator.cc@387 PS5, Line 387: mirroring : // TYPE_STRUCT nit: this also sounds ai-ish http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr.cc@342 PS5, Line 342: && !InvolvesVariantType(); Can we somehow get away without checking the siblings? Like providing a call to the interpreted path instead. That way the InvolvesVariantType is not needed. http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/slot-ref.cc File be/src/exprs/slot-ref.cc: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/slot-ref.cc@591 PS5, Line 591: // A VARIANT slot is two consecutive StringValues: metadata at slot_offset_ and value : // at slot_offset_ + sizeof(StringValue). Convert each to a UDF StringVal. nit: it's commented everywhere, I think it's enough to emphasize it on the type definition http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/variant-functions.h File be/src/exprs/variant-functions.h: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/variant-functions.h@54 PS5, Line 54: class VariantFunctions { I think there's room for templating here, that brings in mangled names for builtin registration, but maybe yields less code. What's your opinion? Update: I saw the macros, I tend to go with templates instead -- To view, visit http://gerrit.cloudera.org:8080/24557 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55bfed394ac2fb57135fadedd489f59e4cc10de4 Gerrit-Change-Number: 24557 Gerrit-PatchSet: 5 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Thu, 27 Aug 2026 19:28:24 +0000 Gerrit-HasComments: Yes
