Daniel Vanko 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/24557/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24557/7//COMMIT_MSG@48 PS7, Line 48: Assisted-by: Claude Opus 4.8 (1M context) <[email protected]> nit: use the Assisted-by: <model> (<agent-name>) format instead http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-ir.cc File be/src/exprs/variant-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-ir.cc@49 PS7, Line 49: StringVal VariantFunctions::VariantToJson(FunctionContext* ctx, const VariantVal& v) { : if (v.is_null || v.metadata.is_null || v.value.is_null) return StringVal::null(); : StringVal result; : Status status = impala::VariantToJson(ctx, v.metadata.ptr, v.metadata.len, : v.value.ptr, v.value.len, &result); : if (!status.ok()) return StringVal::null(); : return result; : } Can we reuse the previous function? Like this: StringVal VariantFunctions::VariantToJson(FunctionContext* ctx, const VariantVal& v) { if (v.is_null) return StringVal::null(); return VariantFunctions::VariantToJson(ctx, v.metadata, v.value); } http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-ir.cc@70 PS7, Line 70: Prepare doesn't exist anymore http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-ir.cc@126 PS7, Line 126: int8_t t; if (!sub.GetInt8(&t)) return Coerce::MISMATCH; *out = t; For me, these formats are hard to read. Maybe split into multiple rows? http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-test.cc File be/src/exprs/variant-functions-test.cc: http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-test.cc@198 PS7, Line 198: std::vector<FunctionContext*> owned_ctxs_; Is there a reason why this is placed here? http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-test.cc@466 PS7, Line 466: BUILD_PERSON(); Add a test with malformed path. http://gerrit.cloudera.org:8080/#/c/24557/7/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/24557/7/common/function-registry/impala_functions.py@1202 PS7, Line 1202: FunctionCallExpr chooses the : # return type from the literal type tag and resolves the matching internal overload. : # Distinct names are required because the catalog cannot hold multiple functions with : # the same (VARIANT,STRING,STRING) signature differing only by return type. I think it's unnecessary, because it's true for all other functions. http://gerrit.cloudera.org:8080/#/c/24557/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant-get.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant-get.test: http://gerrit.cloudera.org:8080/#/c/24557/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-v3-variant-get.test@421 PS7, Line 421: # Constant path must be '$'-rooted. What about malformed paths like '$age'? -- 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: Daniel Vanko <[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: Tue, 08 Sep 2026 17:46:21 +0000 Gerrit-HasComments: Yes
