Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24871 )
Change subject: IMPALA-15203: Add aggregate function support for UUID columns ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/24871/5/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: http://gerrit.cloudera.org:8080/#/c/24871/5/be/src/exprs/agg-fn-evaluator.cc@308 PS5, Line 308: if (type.type == TYPE_CHAR || type.type == TYPE_FIXED_UDA_INTERMEDIATE does this change alter the behavior of existing types TYPE_CHAR and TYPE_FIXED_UDA_INTERMEDIATE? if yes, then it could be mentioned in commit message http://gerrit.cloudera.org:8080/#/c/24871/5/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/24871/5/be/src/exprs/aggregate-functions-ir.cc@1314 PS5, Line 1314: template<> : void AggregateFunctions::Max(FunctionContext* ctx, const StringVal& src, StringVal* dst) { : if (src.is_null) return; : if (dst->is_null || : StringValue::FromStringVal(src) > StringValue::FromStringVal(*dst)) { : if (!dst->is_null) ctx->Free(dst->ptr); : CopyStringVal(ctx, src, dst); : } : } : : void AggregateFunctions::MinUuid(FunctionContext*, const StringVal& src, StringVal* dst) { : if (src.is_null) return; : DCHECK_EQ(src.len, UUID_BYTE_LEN); : if (dst->is_null || memcmp(src.ptr, dst->ptr, UUID_BYTE_LEN) < 0) { : memcpy(dst->ptr, src.ptr, UUID_BYTE_LEN); : dst->len = UUID_BYTE_LEN; : dst->is_null = false; : } : } I was thinking about why string and uuid functions are different at all, then realized that the string functions are actually pretty suboptimal for the (probably common) case when the length of src and dst are equal, as they do free + allocate while they could overwrite dst's buffer. Probably not in this commit, but it would be nice to optimize this case for strings too. http://gerrit.cloudera.org:8080/#/c/24871/5/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/24871/5/tests/query_test/test_iceberg.py@2722 PS5, Line 2722: create_iceberg_table_from_directory(self.client, unique_database, : "iceberg_uuid_test", "parquet", : table_location="${IMPALA_HOME}/testdata/data/iceberg_test/iceberg_uuid") Can't we use this during dataload ( functional_schema_template.sql)? Currently I don't see any uuid columns in our data load, which makes experimentation harder in my experience. -- To view, visit http://gerrit.cloudera.org:8080/24871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2187495c8be5b95ecff3f458b6549916466e9c83 Gerrit-Change-Number: 24871 Gerrit-PatchSet: 5 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 25 Sep 2026 06:22:11 +0000 Gerrit-HasComments: Yes
