dramaticlly commented on code in PR #17413:
URL: https://github.com/apache/iceberg/pull/17413#discussion_r3678726751
##########
format/spec.md:
##########
@@ -799,6 +799,8 @@ In Iceberg v4, statistics are stored in typed fields
grouped in a struct that co
Field-level structs in `content_stats` are based on the corresponding table
field's type, requirement, and ID (`field-id`).
+Stats are tracked for primitive and `variant` fields. Stats are not tracked
for `struct`, `list`, and `map` fields, or for any field contained in a `list`
or `map` because it may be repeated.
Review Comment:
maybe consider move the spec to a separate PR?
##########
core/src/main/java/org/apache/iceberg/StatsUtil.java:
##########
@@ -317,6 +324,20 @@ static Types.StructType fieldStatsStruct(
return Types.StructType.of(fields);
}
+ /** Return whether a field may be null, either because it or any ancestor is
optional. */
+ private static boolean isNullable(Schema schema, Map<Integer, Integer>
parentIndex, int id) {
+ Integer currentId = id;
+ while (currentId != null) {
+ if (schema.findField(currentId).isOptional()) {
+ return true;
+ }
+
+ currentId = parentIndex.get(currentId);
+ }
+
+ return false;
+ }
Review Comment:
I think this method is very similar to
InclusiveStatsEvaluator::allAncestorFieldsAreRequired, since both are purely
schema based and have nothing to do with stats, maybe consider move to the
schema.isNullable(int fieldId) instead as it already have the fieldId of all
schema cached lazily?
In reader, it can use `referencedIds.stream().filter(id ->
!schema.isNullable(id))` and in writer it can be
`fieldStatsStruct(tableSchema.isNullable(id), ...)`.
##########
format/spec.md:
##########
@@ -820,10 +822,12 @@ Each stats struct holds statistics for one table field.
It may contain the follo
| _optional_ | 2 | `upper_bound` | Field type or `geo_upper`
| all primitives or `variant` | Upper bound stored as the
field's type, or `geo_upper` for geo types |
| _optional_ | 3 | `tight_bounds` | `boolean`
| all except `geometry`, `geography`, `variant` | When true, `lower_bound` and
`upper_bound` must be equal to the min and max values |
| _optional_ | 4 | `value_count` | `long`
| all | Number of values in the
column (including null and NaN values) |
-| _optional_ | 5 | `null_value_count` | `long`
| optional fields | Number of null values in the
column |
+| _optional_ | 5 | `null_value_count` | `long`
| fields that may be null | Number of null values in the
column |
Review Comment:
looks like we now omit the fields which is definitely required, null count
is guaranteed to be 0. Curious, is this intentional depart from v1-v3
null_count?
##########
core/src/test/java/org/apache/iceberg/TestInclusiveStatsEvaluator.java:
##########
@@ -0,0 +1,252 @@
+/*
+ * 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;
+
+import static org.apache.iceberg.expressions.Expressions.lessThan;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.List;
+import org.apache.iceberg.expressions.Expression;
+import org.apache.iceberg.expressions.InclusiveStatsEvaluator;
+import org.apache.iceberg.expressions.TestInclusiveMetricsEvaluator;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Test;
+
+/** Runs the inclusive evaluation tests against {@link ContentStats} tracked
by a file. */
Review Comment:
nit: probably don't need this
--
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]
