aihuaxu commented on code in PR #13137: URL: https://github.com/apache/iceberg/pull/13137#discussion_r2118091704
########## api/src/main/java/org/apache/iceberg/expressions/ExpressionUtil.java: ########## @@ -646,6 +654,86 @@ private static String sanitizeSimpleString(CharSequence value) { return String.format(Locale.ROOT, "(hash-%08x)", HASH_FUNC.apply(value)); } + private static String sanitizeVariant(VariantData value, long now, int today) { + return sanitizeVariant(value.value(), now, today); + } + + private static String sanitizeVariant(VariantValue value, long now, int today) { + if (value instanceof VariantObject) { + return sanitizeVariantObject(value.asObject(), now, today); + } else { + return sanitizeVariantArray(value.asArray(), now, today); + } Review Comment: How about primitive type? VariantValue can be VariantPrimitive. ########## api/src/main/java/org/apache/iceberg/expressions/Literal.java: ########## @@ -71,6 +72,10 @@ static Literal<BigDecimal> of(BigDecimal value) { return new Literals.DecimalLiteral(value); } + static Literal<VariantData> of(VariantData value) { Review Comment: You should use Variant interface instead of VariantData. So you don't need to expose VariantData as public class. ########## api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java: ########## @@ -1050,6 +1057,53 @@ public void testSelectsPartitions() { .isFalse(); } + @Test + public void testSanitizeVariant() { Review Comment: Can we test out variant primitive, variant array in addition to variant object? ########## api/src/main/java/org/apache/iceberg/variants/VariantData.java: ########## @@ -20,7 +20,7 @@ import org.apache.iceberg.relocated.com.google.common.base.Preconditions; -class VariantData implements Variant { +public class VariantData implements Variant { Review Comment: We don't want to expose this as public. ########## api/src/test/java/org/apache/iceberg/variants/VariantTestUtil.java: ########## @@ -107,6 +130,124 @@ static SerializedPrimitive createString(String string) { return SerializedPrimitive.from(buffer, buffer.get(0)); } + public static SerializedArray createMixedArray() { + ByteBuffer nestedBuffer = VariantTestUtil.createArray(A, C, D); Review Comment: I think VariantTestUtil is just a util class with the input provided externally. We don't want to provide a hard coded createINt16() e.g., here since in the test, how can you check and compare the expected value. Better to move to the test itself. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org