Daniel Vanko has posted comments on this change. ( http://gerrit.cloudera.org:8080/23783 )
Change subject: IMPALA-14321: Add BINARY partition transform to Iceberg tables ...................................................................... Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/23783/10/be/src/exprs/iceberg-functions-test.cc File be/src/exprs/iceberg-functions-test.cc: http://gerrit.cloudera.org:8080/#/c/23783/10/be/src/exprs/iceberg-functions-test.cc@237 PS10, Line 237: for (size_t width = 1; width <= sizeof(binary_data); ++width) { : StringVal ret_val = IcebergFunctions::TruncatePartitionTransform<true>( : ctx, input, IntVal(width)); : EXPECT_EQ(ret_val.len, width); : EXPECT_EQ(memcmp((char*)input.ptr, (char*)ret_val.ptr, width), 0 > You could write a for loop with width 1..sizeof(binary_data). Done http://gerrit.cloudera.org:8080/#/c/23783/10/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/23783/10/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@862 PS10, Line 862: set = StandardCharsets.UTF_ > nit: we could have a static variable CHARSET_FOR_BINARY = StandardCharsets. Done http://gerrit.cloudera.org:8080/#/c/23783/10/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1220 PS10, Line 1220: TextConverter::WriteSlot in be/src/exec/text-conve > Elsewhere we use StandardCharsets.ISO_8859_1. Any reason to use Base64 enco Yes, in FileMetadataUtils::AddIcebergColumns https://gerrit.cloudera.org/c/23783/11/be/src/exec/file-metadata-utils.cc#133 when TextConverter::WriteSlot https://gerrit.cloudera.org/c/23783/10/be/src/exec/text-converter.inline.h#70 wants to fill out the slot, it expects binary values to be base64 encoded. Added this as a comment. http://gerrit.cloudera.org:8080/#/c/23783/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test: http://gerrit.cloudera.org:8080/#/c/23783/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-show-files-partition.test@315 PS10, Line 315: / > Can we include the partition transform value here as well? Done -- To view, visit http://gerrit.cloudera.org:8080/23783 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5fd1ef382aa064dad55445dea00fbd39caeca1d3 Gerrit-Change-Number: 23783 Gerrit-PatchSet: 11 Gerrit-Owner: Daniel Vanko <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 14 Jan 2026 16:13:35 +0000 Gerrit-HasComments: Yes
