stevenzwu commented on code in PR #13804:
URL: https://github.com/apache/iceberg/pull/13804#discussion_r2277754140
##########
api/src/main/java/org/apache/iceberg/Accessors.java:
##########
@@ -59,11 +59,13 @@ private static class PositionAccessor implements
Accessor<StructLike> {
private final int position;
private final Type type;
private final Class<?> javaClass;
+ private final boolean hasOptionalFieldInPath;
- PositionAccessor(int pos, Type type) {
+ PositionAccessor(int pos, boolean isOptional, Type type) {
Review Comment:
nit: add new arg in the end for consistency as your other changes
##########
api/src/test/java/org/apache/iceberg/expressions/TestBoundReferenceProducesNull.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.expressions;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.iceberg.Accessor;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBoundReferenceProducesNull {
+ @Test
+ public void testNonNestedField() {
+ // schema: required, optional
+ Schema schema =
+ new Schema(
+ required(1, "required", Types.IntegerType.get()),
+ optional(2, "optional", Types.IntegerType.get()));
+
+ Types.NestedField requiredField = schema.findField(1);
+ Accessor<StructLike> requiredAccessor = schema.accessorForField(1);
+
+ BoundReference<Integer> requiredRef =
+ new BoundReference<>(requiredField, requiredAccessor, "required");
+ assertThat(requiredRef.producesNull()).isFalse();
+
+ Types.NestedField optionalField = schema.findField(2);
+ Accessor<StructLike> optionalAccessor = schema.accessorForField(2);
+
+ BoundReference<Integer> optionalRef =
+ new BoundReference<>(optionalField, optionalAccessor, "optional");
+ assertThat(optionalRef.producesNull()).isTrue();
+ }
+
+ @Test
+ public void testRequiredLeafNoOptionalAncestors() {
+ // schema: requiredAncestor -> requiredLeaf
+ Schema schema =
+ new Schema(
+ required(
+ 1,
+ "requiredAncestor",
+ Types.StructType.of(required(2, "requiredLeaf",
Types.IntegerType.get()))));
+
+ Types.NestedField requiredLeafField = schema.findField(2);
+ Accessor<StructLike> accessor = schema.accessorForField(2);
+
+ BoundReference<Integer> ref = new BoundReference<>(requiredLeafField,
accessor, "requiredLeaf");
+ assertThat(ref.producesNull()).isFalse();
+ }
+
+ @Test
+ public void testOptionalLeaf() {
+ // schema: requiredAncestor -> optionalLeaf
+ Schema schema =
+ new Schema(
+ required(
+ 1,
+ "requiredAncestor",
+ Types.StructType.of(optional(2, "optionalLeaf",
Types.IntegerType.get()))));
+
+ Types.NestedField optionalLeafField = schema.findField(2);
+ Accessor<StructLike> accessor = schema.accessorForField(2);
+
+ BoundReference<Integer> ref = new BoundReference<>(optionalLeafField,
accessor, "optionalLeaf");
+ assertThat(ref.producesNull()).isTrue();
+ }
+
+ @Test
+ public void testRequiredLeafWithOptionalTopAncestor() {
+ // schema: optionalAncestor -> requiredLeaf
+ Schema schema =
+ new Schema(
+ optional(
+ 1,
+ "optionalAncestor",
+ Types.StructType.of(required(2, "requiredLeaf",
Types.IntegerType.get()))));
+
+ Types.NestedField requiredLeafField = schema.findField(2);
+ Accessor<StructLike> accessor = schema.accessorForField(2);
+
+ BoundReference<Integer> ref = new BoundReference<>(requiredLeafField,
accessor, "requiredLeaf");
+ assertThat(ref.producesNull()).isTrue();
+ }
+
+ @Test
+ public void testRequiredLeafWithOptionalIntermediateAncestor() {
+ // schema: requiredRoot -> optionalIntermediate -> requiredIntermediate ->
requiredLeaf
+ Schema schema =
+ new Schema(
+ required(
+ 1,
+ "requiredRoot",
Review Comment:
nit: following the style from `TestAccessors`, can we call the struct field
name as s1, s2, s3 etc. where the number indicates the nested level? for
optional, maybe name it as `s2_optional`.
##########
api/src/test/java/org/apache/iceberg/expressions/TestBoundReferenceProducesNull.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.expressions;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.iceberg.Accessor;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBoundReferenceProducesNull {
Review Comment:
nit: call it `TestBoundedReference`?
##########
.palantir/revapi.yml:
##########
@@ -1254,6 +1254,10 @@ acceptedBreaks:
new: "method void
org.apache.iceberg.data.parquet.BaseParquetWriter<T>::<init>()"
justification: "Changing deprecated code"
"1.9.0":
+ org.apache.iceberg:iceberg-api:
+ - code: "java.method.addedToInterface"
+ new: "method boolean
org.apache.iceberg.Accessor<T>::hasOptionalFieldInPath()"
+ justification: "All subclasses implement name"
Review Comment:
nit: copy and paste error on `name`. maybe just `All subclasses implement
the new method`?
##########
parquet/src/test/java/org/apache/iceberg/parquet/TestBloomRowGroupFilter.java:
##########
@@ -322,9 +320,7 @@ public void testIsNull() {
shouldRead =
new ParquetBloomRowGroupFilter(SCHEMA,
isNull("struct_not_null.int_field"))
.shouldRead(parquetSchema, rowGroupMetadata, bloomStore);
- assertThat(shouldRead)
- .as("Should skip: this field is required and are always not-null")
- .isFalse();
+ assertThat(shouldRead).as("Should read: bloom filter doesn't
help").isTrue();
Review Comment:
nit: maybe add a comment line that explains the scenario of required nested
field inside an optional struct can still be evaluated to null.
##########
api/src/main/java/org/apache/iceberg/expressions/BoundReference.java:
##########
@@ -57,7 +57,9 @@ public Type type() {
@Override
public boolean producesNull() {
- return field.isOptional();
+ // A leaf required field can evaluate to null if it is optional itself or
any
+ // ancestor on the path is optional.
+ return field.isOptional() || accessor.hasOptionalFieldInPath();
Review Comment:
`field.isOptional()` is not needed anymore? it should be already covered by
the `accessor.hasOptionalFieldInPath()`?
##########
api/src/test/java/org/apache/iceberg/expressions/TestBoundReferenceProducesNull.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * 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.expressions;
+
+import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.apache.iceberg.Accessor;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+public class TestBoundReferenceProducesNull {
+ @Test
+ public void testNonNestedField() {
+ // schema: required, optional
+ Schema schema =
+ new Schema(
+ required(1, "required", Types.IntegerType.get()),
+ optional(2, "optional", Types.IntegerType.get()));
+
+ Types.NestedField requiredField = schema.findField(1);
+ Accessor<StructLike> requiredAccessor = schema.accessorForField(1);
+
+ BoundReference<Integer> requiredRef =
+ new BoundReference<>(requiredField, requiredAccessor, "required");
+ assertThat(requiredRef.producesNull()).isFalse();
+
+ Types.NestedField optionalField = schema.findField(2);
+ Accessor<StructLike> optionalAccessor = schema.accessorForField(2);
+
+ BoundReference<Integer> optionalRef =
+ new BoundReference<>(optionalField, optionalAccessor, "optional");
+ assertThat(optionalRef.producesNull()).isTrue();
+ }
+
+ @Test
+ public void testRequiredLeafNoOptionalAncestors() {
+ // schema: requiredAncestor -> requiredLeaf
+ Schema schema =
+ new Schema(
+ required(
+ 1,
+ "requiredAncestor",
+ Types.StructType.of(required(2, "requiredLeaf",
Types.IntegerType.get()))));
+
+ Types.NestedField requiredLeafField = schema.findField(2);
+ Accessor<StructLike> accessor = schema.accessorForField(2);
+
+ BoundReference<Integer> ref = new BoundReference<>(requiredLeafField,
accessor, "requiredLeaf");
+ assertThat(ref.producesNull()).isFalse();
+ }
+
+ @Test
+ public void testOptionalLeaf() {
+ // schema: requiredAncestor -> optionalLeaf
+ Schema schema =
+ new Schema(
+ required(
+ 1,
+ "requiredAncestor",
+ Types.StructType.of(optional(2, "optionalLeaf",
Types.IntegerType.get()))));
+
+ Types.NestedField optionalLeafField = schema.findField(2);
+ Accessor<StructLike> accessor = schema.accessorForField(2);
+
+ BoundReference<Integer> ref = new BoundReference<>(optionalLeafField,
accessor, "optionalLeaf");
+ assertThat(ref.producesNull()).isTrue();
+ }
+
+ @Test
+ public void testRequiredLeafWithOptionalTopAncestor() {
+ // schema: optionalAncestor -> requiredLeaf
+ Schema schema =
+ new Schema(
+ optional(
+ 1,
+ "optionalAncestor",
+ Types.StructType.of(required(2, "requiredLeaf",
Types.IntegerType.get()))));
+
+ Types.NestedField requiredLeafField = schema.findField(2);
+ Accessor<StructLike> accessor = schema.accessorForField(2);
+
+ BoundReference<Integer> ref = new BoundReference<>(requiredLeafField,
accessor, "requiredLeaf");
+ assertThat(ref.producesNull()).isTrue();
+ }
+
+ @Test
+ public void testRequiredLeafWithOptionalIntermediateAncestor() {
+ // schema: requiredRoot -> optionalIntermediate -> requiredIntermediate ->
requiredLeaf
+ Schema schema =
+ new Schema(
+ required(
+ 1,
+ "requiredRoot",
+ Types.StructType.of(
+ optional(
+ 2,
+ "optionalIntermediate",
+ Types.StructType.of(
+ required(
+ 3,
+ "requiredIntermediate",
+ Types.StructType.of(
+ required(4, "requiredLeaf",
Types.IntegerType.get()))))))));
+
+ Types.NestedField requiredLeafField = schema.findField(4);
+ Accessor<StructLike> accessor = schema.accessorForField(4);
+
+ BoundReference<Integer> ref = new BoundReference<>(requiredLeafField,
accessor, "requiredLeaf");
+ assertThat(ref.producesNull()).isTrue();
+ }
+}
Review Comment:
let's also add a test with 4 nested level. see `TestAccessors.nested4` so
that `WrapperPositionAccessor` is also covered.
--
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]
