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 8: (8 comments) Thanks for the 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 (Claude Code) > nit: use the Assisted-by: <model> (<agent-name>) format instead Done 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) return StringVal::null(); : return VariantFunctions::VariantToJson(ctx, v.metadata, v.value); : } : : namespace { : : / > Can we reuse the previous function? Like this: Good catch, done. http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-ir.cc@70 PS7, Line 70: > doesn't exist anymore Done http://gerrit.cloudera.org:8080/#/c/24557/7/be/src/exprs/variant-functions-ir.cc@126 PS7, Line 126: > For me, these formats are hard to read. Maybe split into multiple rows? Done 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? We usually place member fields last in C++ classes. 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. Done 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. : [['variant_get_boolean'], 'BOOLEAN', ['VARIANT', 'STRING', 'STRING'], : 'impala::VariantFunctions::VariantGetBoolean'], > I think it's unnecessary, because it's true for all other functions. Removed the last half. 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'? Implemented stricter path-chek in the FE and added tests. -- 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: 8 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: Fri, 11 Sep 2026 13:50:04 +0000 Gerrit-HasComments: Yes
