huaxingao commented on code in PR #16198: URL: https://github.com/apache/iceberg/pull/16198#discussion_r4428624300
########## api/src/main/java/org/apache/iceberg/functions/Sha256QueryLocal.java: ########## @@ -0,0 +1,71 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.functions; + +import java.io.ObjectStreamException; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.types.Type; +import org.apache.iceberg.util.SerializableFunction; + +/** + * Hashes values with SHA-256 using a per-query salt. Binding requires the salt via {@link + * #bind(Type, byte[])}; the no-salt variant fails fast so callers can't accidentally strip the + * query-local randomness. + */ +final class Sha256QueryLocal<T> implements SaltedFunction<T, T> { + static final String NAME = "sha_256_query_local"; + + private static final Sha256QueryLocal<?> INSTANCE = new Sha256QueryLocal<>(); + + @SuppressWarnings("unchecked") + static <T> Sha256QueryLocal<T> get() { + return (Sha256QueryLocal<T>) INSTANCE; + } + + private Sha256QueryLocal() {} + + @Override + public String toString() { + return NAME; + } + + Object writeReplace() throws ObjectStreamException { + return SerializationProxies.Sha256QueryLocalProxy.get(); + } + + @Override + public boolean canBind(Type type) { + return Sha256.isSupported(type); + } + + @Override + public SerializableFunction<T, T> bind(Type type) { + throw new IllegalArgumentException( + "sha_256_query_local requires a salt; call bind(Type, byte[]) instead"); Review Comment: nit: nothing is wrong with the argument here; this method just can't be used without a salt. Should this be `UnsupportedOperationException`, like the default `IcebergFunction.bind` and `UnknownTransform.bind`? ########## api/src/main/java/org/apache/iceberg/functions/IcebergFunctions.java: ########## @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.functions; + +/** Factory for the {@link IcebergFunction} implementations defined by the REST spec. */ +public class IcebergFunctions { + private IcebergFunctions() {} + + /** + * Returns the function with the given name, or an {@link UnknownFunction} if the name is not + * recognized. + */ + public static IcebergFunction<?, ?> fromString(String function) { Review Comment: The REST spec uses kebab-case names (`show-last-4`), but the names here are snake_case (`show_last_4`). So if we call `fromString("show-last-4")`, it returns `UnknownFunction` and the query would fail. We need to convert the REST name to this name somewhere. Where should this happen: in the REST parser (next PR), or here in `fromString`? Also, the javadoc in `IcebergFunctions`/`UnknownFunction` says these are the names "defined by the REST spec", but `fromString` only accepts the snake_case names. Can we update the javadoc to say this clearly? ########## api/src/main/java/org/apache/iceberg/functions/ShowFirst4.java: ########## @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.functions; + +import java.io.ObjectStreamException; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.types.Type; +import org.apache.iceberg.util.SerializableFunction; + +/** Preserves the first 4 code points of a string, redacts the rest via mask_alphanum rules. */ +final class ShowFirst4 implements IcebergFunction<CharSequence, CharSequence> { + static final String NAME = "show_first_4"; + + private static final ShowFirst4 INSTANCE = new ShowFirst4(); + + static ShowFirst4 get() { + return INSTANCE; + } + + private ShowFirst4() {} + + @Override + public String toString() { + return NAME; + } + + Object writeReplace() throws ObjectStreamException { + return SerializationProxies.ShowFirst4Proxy.get(); + } + + @Override + public boolean canBind(Type type) { + return type.typeId() == Type.TypeID.STRING; + } + + @Override + public SerializableFunction<CharSequence, CharSequence> bind(Type type) { + Preconditions.checkArgument(canBind(type), "show_first_4 requires STRING type, got %s", type); + return ShowFirst4Fn.INSTANCE; + } + + private static final class ShowFirst4Fn extends NullSafeFunction<CharSequence, CharSequence> { + static final ShowFirst4Fn INSTANCE = new ShowFirst4Fn(); + + @Override + protected CharSequence applyNonNull(CharSequence input) { + if (Character.codePointCount(input, 0, input.length()) <= 4) { + return input; Review Comment: nit: for 4 or fewer code points this returns the input object, so a `StringBuilder` input comes back as `StringBuilder`, while longer input comes back as `String`. Should we return `input.toString()` to be consistent with `MaskAlphanum`? Same in `ShowLast4`. ########## api/src/test/java/org/apache/iceberg/functions/TestIcebergFunctions.java: ########## @@ -0,0 +1,481 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.functions; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.math.BigDecimal; +import java.nio.ByteBuffer; +import java.time.LocalDate; +import java.time.LocalDateTime; +import java.time.ZoneOffset; +import java.util.Arrays; +import org.apache.iceberg.StructLike; +import org.apache.iceberg.types.Types; +import org.apache.iceberg.util.DateTimeUtil; +import org.apache.iceberg.util.SerializableFunction; +import org.junit.jupiter.api.Test; + +public class TestIcebergFunctions { + + @Test + public void maskAlphanumSpecExample() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.maskAlphanum().bind(Types.StringType.get()); + assertThat(fn.apply("[email protected]")).isEqualTo("[email protected]"); + } + + @Test + public void maskAlphanumPreservedPunctuation() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.maskAlphanum().bind(Types.StringType.get()); + assertThat(fn.apply("(555) 123-4567")).isEqualTo("(nnn)xnnn-nnnn"); + assertThat(fn.apply("a.b,c")).isEqualTo("x.x,x"); + } + + @Test + public void maskAlphanumNullInNullOut() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.maskAlphanum().bind(Types.StringType.get()); + assertThat(fn.apply(null)).isNull(); + } + + @Test + public void maskAlphanumEmptyString() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.maskAlphanum().bind(Types.StringType.get()); + assertThat(fn.apply("")).isEqualTo(""); + } + + @Test + public void stringFunctionsAcceptAnyCharSequence() { + CharSequence email = new StringBuilder("[email protected]"); + assertThat(IcebergFunctions.maskAlphanum().bind(Types.StringType.get()).apply(email)) + .hasToString("[email protected]"); + assertThat(IcebergFunctions.showFirst4().bind(Types.StringType.get()).apply(email)) + .hasToString("[email protected]"); + assertThat(IcebergFunctions.showLast4().bind(Types.StringType.get()).apply(email)) + .hasToString("[email protected]"); + assertThat(IcebergFunctions.sha256Global().bind(Types.StringType.get()).apply(email)) + .isEqualTo( + IcebergFunctions.sha256Global() + .bind(Types.StringType.get()) + .apply("[email protected]")); + } + + @Test + public void showFirst4SpecExample() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.showFirst4().bind(Types.StringType.get()); + assertThat(fn.apply("[email protected]")).isEqualTo("[email protected]"); + } + + @Test + public void showFirst4FourOrFewerReturnedUnchanged() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.showFirst4().bind(Types.StringType.get()); + assertThat(fn.apply("abcd")).isEqualTo("abcd"); + assertThat(fn.apply("ab")).isEqualTo("ab"); + assertThat(fn.apply("")).isEqualTo(""); + } + + @Test + public void showLast4SpecExample() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.showLast4().bind(Types.StringType.get()); + assertThat(fn.apply("4111-1111-1111-4444")).isEqualTo("nnnn-nnnn-nnnn-4444"); + } + + @Test + public void showLast4FourOrFewerReturnedUnchanged() { + SerializableFunction<CharSequence, CharSequence> fn = + IcebergFunctions.showLast4().bind(Types.StringType.get()); + assertThat(fn.apply("abcd")).isEqualTo("abcd"); + assertThat(fn.apply("ab")).isEqualTo("ab"); + } + + @Test + public void replaceWithNullAlwaysReturnsNull() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.replaceWithNull().bind(Types.IntegerType.get()); + assertThat(fn.apply(42)).isNull(); + assertThat(fn.apply(null)).isNull(); + + SerializableFunction<Object, Object> strFn = + IcebergFunctions.replaceWithNull().bind(Types.StringType.get()); + assertThat(strFn.apply("hello")).isNull(); + } + + @Test + public void maskToFixedValueString() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.StringType.get()); + assertThat(fn.apply("anything")).isEqualTo("XXXXXXXX"); + } + + @Test + public void maskToFixedValueInt() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.IntegerType.get()); + assertThat(fn.apply(42)).isEqualTo(0); + } + + @Test + public void maskToFixedValueLong() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.LongType.get()); + assertThat(fn.apply(42L)).isEqualTo(0L); + } + + @Test + public void maskToFixedValueDouble() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.DoubleType.get()); + assertThat(fn.apply(3.14)).isEqualTo(0.0d); + } + + @Test + public void maskToFixedValueBoolean() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.BooleanType.get()); + assertThat(fn.apply(true)).isEqualTo(false); + } + + @Test + public void maskToFixedValueDate() { + int input = DateTimeUtil.daysFromDate(LocalDate.of(2024, 7, 15)); + int expected = DateTimeUtil.daysFromDate(LocalDate.of(1970, 1, 1)); + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.DateType.get()); + assertThat(fn.apply(input)).isEqualTo(expected); + } + + @Test + public void maskToFixedValueTimestamp() { + long input = + LocalDateTime.of(2024, 7, 15, 13, 45, 30).toEpochSecond(ZoneOffset.UTC) * 1_000_000L; + long expected = LocalDateTime.of(1970, 1, 1, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1_000_000L; + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.TimestampType.withZone()); + assertThat(fn.apply(input)).isEqualTo(expected); + } + + @Test + public void maskToFixedValueBinary() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.BinaryType.get()); + ByteBuffer result = (ByteBuffer) fn.apply(ByteBuffer.wrap(new byte[] {1, 2, 3})); + assertThat(result.remaining()).isEqualTo(0); + } + + @Test + public void maskToFixedValueStructWithBinaryField() { + Types.StructType struct = + Types.StructType.of( + Types.NestedField.optional(1, "id", Types.IntegerType.get()), + Types.NestedField.optional(2, "payload", Types.BinaryType.get())); + SerializableFunction<Object, Object> fn = IcebergFunctions.maskToFixedValue().bind(struct); + StructLike result = (StructLike) fn.apply(null); + assertThat(result.get(0, Integer.class)).isEqualTo(0); + assertThat(result.get(1, ByteBuffer.class).remaining()).isEqualTo(0); + } + + @Test + public void maskToFixedValueDecimal() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.DecimalType.of(10, 2)); + BigDecimal result = (BigDecimal) fn.apply(new BigDecimal("12.34")); + assertThat(result.compareTo(BigDecimal.ZERO)).isEqualTo(0); + assertThat(result.scale()).isEqualTo(2); + } + + @Test + public void maskToFixedValueCanBindStructOfSupportedTypes() { + Types.StructType struct = + Types.StructType.of( + Types.NestedField.optional(1, "id", Types.IntegerType.get()), + Types.NestedField.optional( + 2, + "address", + Types.StructType.of( + Types.NestedField.optional(3, "city", Types.StringType.get())))); + assertThat(IcebergFunctions.maskToFixedValue().canBind(struct)).isTrue(); + } + + @Test + public void maskToFixedValueCannotBindStructWithUnsupportedNestedField() { + Types.StructType struct = + Types.StructType.of( + Types.NestedField.optional(1, "id", Types.IntegerType.get()), + Types.NestedField.optional( + 2, + "address", + Types.StructType.of( + Types.NestedField.optional(3, "location", Types.GeometryType.crs84())))); + assertThat(IcebergFunctions.maskToFixedValue().canBind(struct)).isFalse(); + assertThatThrownBy(() -> IcebergFunctions.maskToFixedValue().bind(struct)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("mask_to_fixed_value is not supported for type: " + struct); + } + + @Test + public void maskToFixedValueNullReturnsFixedValue() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.maskToFixedValue().bind(Types.IntegerType.get()); + assertThat(fn.apply(null)).isEqualTo(0); + } + + @Test + public void truncateToYearDate() { + int input = (int) LocalDate.of(2024, 7, 15).toEpochDay(); + int expected = (int) LocalDate.of(2024, 1, 1).toEpochDay(); + SerializableFunction<Object, Object> fn = + IcebergFunctions.truncateToYear().bind(Types.DateType.get()); + assertThat(fn.apply(input)).isEqualTo(expected); + } + + @Test + public void truncateToMonthDate() { + int input = (int) LocalDate.of(2024, 7, 15).toEpochDay(); + int expected = (int) LocalDate.of(2024, 7, 1).toEpochDay(); + SerializableFunction<Object, Object> fn = + IcebergFunctions.truncateToMonth().bind(Types.DateType.get()); + assertThat(fn.apply(input)).isEqualTo(expected); + } + + @Test + public void truncateToYearTimestamp() { + long inputMicros = + LocalDateTime.of(2024, 7, 15, 13, 45, 30).toEpochSecond(ZoneOffset.UTC) * 1_000_000L; + long expectedMicros = + LocalDateTime.of(2024, 1, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1_000_000L; + SerializableFunction<Object, Object> fn = + IcebergFunctions.truncateToYear().bind(Types.TimestampType.withZone()); + assertThat(fn.apply(inputMicros)).isEqualTo(expectedMicros); + } + + @Test + public void truncateToMonthTimestamp() { + long inputMicros = + LocalDateTime.of(2024, 7, 15, 13, 45, 30).toEpochSecond(ZoneOffset.UTC) * 1_000_000L; + long expectedMicros = + LocalDateTime.of(2024, 7, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1_000_000L; + SerializableFunction<Object, Object> fn = + IcebergFunctions.truncateToMonth().bind(Types.TimestampType.withZone()); + assertThat(fn.apply(inputMicros)).isEqualTo(expectedMicros); + } + + @Test + public void truncateToYearTimestampNanoTz() { + long inputNanos = + LocalDateTime.of(2024, 7, 15, 13, 45, 30).toEpochSecond(ZoneOffset.UTC) * 1_000_000_000L; + long expectedNanos = + LocalDateTime.of(2024, 1, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1_000_000_000L; + SerializableFunction<Object, Object> fn = + IcebergFunctions.truncateToYear().bind(Types.TimestampNanoType.withZone()); + assertThat(fn.apply(inputNanos)).isEqualTo(expectedNanos); + } + + @Test + public void truncateToMonthTimestampNanoTz() { + long inputNanos = + LocalDateTime.of(2024, 7, 15, 13, 45, 30).toEpochSecond(ZoneOffset.UTC) * 1_000_000_000L; + long expectedNanos = + LocalDateTime.of(2024, 7, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1_000_000_000L; + SerializableFunction<Object, Object> fn = + IcebergFunctions.truncateToMonth().bind(Types.TimestampNanoType.withZone()); + assertThat(fn.apply(inputNanos)).isEqualTo(expectedNanos); + } + + @Test + public void sha256GlobalStringIsDeterministic() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.sha256Global().bind(Types.StringType.get()); + String first = (String) fn.apply("hello"); + String second = (String) fn.apply("hello"); + assertThat(first).isEqualTo(second); + assertThat(first).isEqualTo("2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"); + } + + @Test + public void sha256GlobalBinaryReturns32Bytes() { + SerializableFunction<Object, Object> fn = + IcebergFunctions.sha256Global().bind(Types.BinaryType.get()); + ByteBuffer result = (ByteBuffer) fn.apply(ByteBuffer.wrap(new byte[] {1, 2, 3})); + assertThat(result.remaining()).isEqualTo(32); + } + + @Test + public void sha256GlobalIntegerDeterministic() { Review Comment: Other engines need to produce the same hash, so byte order is important here. These int/long tests only check that the result is the same twice, so they would still pass if the byte order were wrong. Can we compute the expected value in the test (`MessageDigest` over the little-endian bytes, then read the first 4 bytes as a little-endian int)? It would also be good to check that the salt in `sha_256_query_local` is added before the value, not after. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
