Zoltan Borok-Nagy 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 7: (13 comments) Thanks for the 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: dst->is_null = true; > I don't see the added value by this function It felt weird to just add a case TYPE_VARIANT branch to SetAnyVal(), as slot->VariantVal conversion already happened outside of that function. While all other branches do the slot->*Val conversion. 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: if (root.type().IsStructType()) { : DCHECK(root.GetNumChildren() > 0); : Status status = Create(root.children(), state, pool, expr_perm_pool, : expr_results_pool, &((*eval)->childEvaluators_) > This comment might be better placed somewhere near the VariantVal Removed http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr-evaluator.cc@386 PS5, Line 386: : DCHECK(false) > nit: not needed to mention Removed comment http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/scalar-expr-evaluator.cc@387 PS5, Line 387: ring(); : return nullptr > nit: this also sounds ai-ish Done 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 cal I think this would require non-trivial codegen work. I rather just remove this limitation by implementing codegen support for VARIANTs (IMPALA-15141). 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: const uint8_t* slot = reinterpret_cast<const uint8_t*>(t->GetSlot(slot_offset_)); : const StringValue* meta_sv = reinterpret_cast<const StringValue*>(slot); > nit: it's commented everywhere, I think it's enough to emphasize it on the Removed http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/variant-functions-test.cc File be/src/exprs/variant-functions-test.cc: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/exprs/variant-functions-test.cc@119 PS5, Line 119: class VariantFunctionsTest : public ::testing::Test { > Tests could be expanded by: Added new tests 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 I lean towards to keep the readable names like VariantGetBoolean, VariantGetTinyInt, etc. I could still add a common template helper function, replacing the macros, but the total code lines and complexity would increase and I'm not sure that it worth it. http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/service/hs2-util.cc File be/src/service/hs2-util.cc: http://gerrit.cloudera.org:8080/#/c/24557/5/be/src/service/hs2-util.cc@415 PS5, Line 415: A VARIANT always reaches us as a VariantV > nit: it should be only commented on the type definition, same goes for quer Done http://gerrit.cloudera.org:8080/#/c/24557/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/24557/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@856 PS5, Line 856: final boolean isVariantGet = fnName_.isBuiltin() : > This methods feels like it's doing at least 3 different things with argumen The method has been split up. http://gerrit.cloudera.org:8080/#/c/24557/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@872 PS5, Line 872: > nit: StringLiteral stringLiteral Done http://gerrit.cloudera.org:8080/#/c/24557/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@874 PS5, Line 874: n_.get(0). > nit:maybe a nullcheck before? Done http://gerrit.cloudera.org:8080/#/c/24557/5/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@902 PS5, Line 902: String rawTag = typeLiteral.getStringValue(); : String typeTag = rawTag == null ? "" : rawTag.toLowerCase(); : switch (typeTag) { : > It' duplicated from analyzeImpl Done -- 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: 7 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 02 Sep 2026 13:17:06 +0000 Gerrit-HasComments: Yes
