ldudas-marx commented on code in PR #16264: URL: https://github.com/apache/iceberg/pull/16264#discussion_r3233883450
########## arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestNullabilityHolder.java: ########## @@ -0,0 +1,87 @@ +/* + * 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.arrow.vectorized; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +class TestNullabilityHolder { + + @Test + void resetClearsIsNullMarkers() { + // Regression test for #15808: reset() must clear the isNull marker + // array, not only numNulls. Otherwise, callers reusing the holder + // across batches observe stale null markers via isNullAt(). + NullabilityHolder holder = new NullabilityHolder(10); + holder.setNull(3); + + assertThat(holder.hasNulls()).isTrue(); + assertThat(holder.isNullAt(3)).isEqualTo((byte) 1); + + holder.reset(); + + assertThat(holder.hasNulls()).isFalse(); + assertThat(holder.numNulls()).isZero(); + // Before the fix, this assertion failed: the isNull[3] byte still + // held the stale 1 from the previous batch. + assertThat(holder.isNullAt(3)) + .as("isNull marker at index 3 must be cleared by reset()") + .isEqualTo((byte) 0); + + // Spot-check a few other indices to confirm the whole array was cleared. Review Comment: nit: The whole array is tested, so it's not a spot-check ########## arrow/src/main/java/org/apache/iceberg/arrow/vectorized/NullabilityHolder.java: ########## @@ -77,5 +77,11 @@ public int numNulls() { public void reset() { numNulls = 0; + // Also clear the isNull marker array so callers that reuse a + // NullabilityHolder across batches don't observe stale markers from + // a previous batch via isNullAt(). The other two arrays (`nulls`, + // `nonNulls`) are immutable arraycopy source buffers and don't need + // clearing. See #15808. + Arrays.fill(isNull, (byte) 0); Review Comment: Arrays.fill iterates over the array. I think the following is faster: ```suggestion setNonNulls(0, size()); ``` ########## arrow/src/test/java/org/apache/iceberg/arrow/vectorized/TestNullabilityHolder.java: ########## @@ -0,0 +1,87 @@ +/* + * 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.arrow.vectorized; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.jupiter.api.Test; + +class TestNullabilityHolder { + + @Test + void resetClearsIsNullMarkers() { + // Regression test for #15808: reset() must clear the isNull marker + // array, not only numNulls. Otherwise, callers reusing the holder + // across batches observe stale null markers via isNullAt(). + NullabilityHolder holder = new NullabilityHolder(10); + holder.setNull(3); + + assertThat(holder.hasNulls()).isTrue(); + assertThat(holder.isNullAt(3)).isEqualTo((byte) 1); + + holder.reset(); + + assertThat(holder.hasNulls()).isFalse(); + assertThat(holder.numNulls()).isZero(); + // Before the fix, this assertion failed: the isNull[3] byte still Review Comment: This comment is not very useful outside the PR context -- 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]
