stevenzwu commented on code in PR #17159:
URL: https://github.com/apache/iceberg/pull/17159#discussion_r3582805652
##########
core/src/main/java/org/apache/iceberg/ContentStats.java:
##########
@@ -18,13 +18,13 @@
*/
package org.apache.iceberg;
-import java.util.List;
+import java.util.Set;
import org.apache.iceberg.types.Types;
-interface ContentStats extends StructLike {
+interface ContentStats {
/** A list of all the {@link FieldStats} */
- List<FieldStats<?>> fieldStats();
+ Iterable<FieldStats<?>> fieldStats();
Review Comment:
nit: the Javadoc still says `list`
##########
core/src/main/java/org/apache/iceberg/ContentStatsStruct.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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 java.io.Serializable;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+
+class ContentStatsStruct implements ContentStats, StructLike, Serializable {
+ private final Map<Integer, FieldStats<?>> idToFieldStats = Maps.newHashMap();
+ private final Types.StructType struct;
+ private final int[] posToId;
+
+ ContentStatsStruct(Types.StructType struct) {
+ this.struct = struct;
+ this.posToId = posToId(struct);
+ }
+
+ private ContentStatsStruct(ContentStatsStruct toCopy, Set<Integer> fieldIds)
{
+ this(fieldIds != null ? TypeUtil.select(toCopy.struct, fieldIds) :
toCopy.struct);
+ if (fieldIds != null) {
+ for (int fieldId : fieldIds) {
+ idToFieldStats.put(fieldId, toCopy.idToFieldStats.get(fieldId).copy());
+ }
+ } else {
+ for (Map.Entry<Integer, FieldStats<?>> entry :
toCopy.idToFieldStats.entrySet()) {
+ idToFieldStats.put(entry.getKey(), entry.getValue().copy());
+ }
+ }
+ }
+
+ @Override
+ public Iterable<FieldStats<?>> fieldStats() {
+ return idToFieldStats.values();
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public <T> FieldStats<T> statsFor(int id) {
+ return (FieldStats<T>) idToFieldStats.get(id);
+ }
+
+ public <T> void setStats(int id, FieldStats<T> fieldStats) {
+ Preconditions.checkArgument(
+ struct.field(StatsUtil.toBaseId(id)) != null,
+ "Cannot set stats for unknown field ID: %s",
+ id);
+ Preconditions.checkArgument(
+ id == fieldStats.fieldId(),
Review Comment:
since `fieldStats.fieldId()` is already there, maybe we don't need the first
arg of `int id` for this method?
##########
core/src/main/java/org/apache/iceberg/ContentStats.java:
##########
@@ -35,6 +35,12 @@ interface ContentStats extends StructLike {
*/
<T> FieldStats<T> statsFor(int fieldId);
- /** The stats struct holding nested structs with their respective field
stats */
- Types.StructType statsStruct();
+ /** Container struct type containing tracked field-level stats structs. */
+ Types.StructType type();
+
+ /** Returns a copy, deep-copying all field stats. */
+ ContentStats copy();
+
+ /** Returns a copy, of only the selected field stats by ID. */
+ ContentStats copy(Set<Integer> fieldIds);
Review Comment:
is `project` a more accurate method name than `copy` here?
##########
core/src/main/java/org/apache/iceberg/FieldStatsStruct.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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 java.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.ByteBuffers;
+
+class FieldStatsStruct<T> implements FieldStats<T>, StructLike, Serializable {
+ private final Types.StructType struct;
+ private final Type boundType;
+ private final int[] posToOffset;
+ private final int fieldId;
+
+ private Object lowerBound = null;
+ private Object upperBound = null;
+ private boolean tightBounds = false;
+ private Long valueCount = null;
+ private Long nullValueCount = null;
+ private Long nanValueCount = null;
+ private Integer avgValueSize = null;
+
+ FieldStatsStruct(Types.StructType struct) {
+ this.struct = struct;
+ this.posToOffset = posToOffset(struct);
+ this.fieldId = StatsUtil.toFieldId(struct.fields().get(0).fieldId());
+ this.boundType = struct.fieldType("lower_bound");
+ }
+
+ FieldStatsStruct(
+ Types.StructType struct,
+ T lowerBound,
+ T upperBound,
+ boolean tightBounds,
+ long valueCount,
+ long nullValueCount,
+ long nanValueCount,
+ Integer avgValueSize) {
+ this(struct);
+ setLowerBound(lowerBound);
+ setUpperBound(upperBound);
+ this.tightBounds = tightBounds;
+ this.valueCount = valueCount;
+ this.nullValueCount = nullValueCount;
+ this.nanValueCount = nanValueCount;
+ this.avgValueSize = avgValueSize;
+ }
+
+ private FieldStatsStruct(FieldStatsStruct<T> toCopy) {
+ this(toCopy.struct);
+ // bounds are stored using the internal representation, which is a byte
array for binary types
+ this.lowerBound =
+ toCopy.lowerBound instanceof byte[]
+ ? ((byte[]) toCopy.lowerBound).clone()
+ : toCopy.lowerBound;
+ this.upperBound =
+ toCopy.upperBound instanceof byte[]
+ ? ((byte[]) toCopy.upperBound).clone()
+ : toCopy.upperBound;
+ this.tightBounds = toCopy.tightBounds;
+ this.valueCount = toCopy.valueCount;
+ this.nullValueCount = toCopy.nullValueCount;
+ this.nanValueCount = toCopy.nanValueCount;
+ this.avgValueSize = toCopy.avgValueSize;
+ }
+
+ void fromFieldMetrics(FieldMetrics<T> fieldMetrics) {
+ Preconditions.checkArgument(
+ fieldMetrics.id() == fieldId,
+ "Cannot store stats for field ID: %s (expected %s)",
+ fieldMetrics.id(),
+ fieldId);
+
+ setLowerBound(fieldMetrics.lowerBound());
+ setUpperBound(fieldMetrics.upperBound());
+ this.tightBounds = false;
+ this.valueCount = fieldMetrics.valueCount();
+ this.nullValueCount = fieldMetrics.nullValueCount() < 0 ? null :
fieldMetrics.nullValueCount();
Review Comment:
why do we set to `null` for negative source value? won't it cause NPE in
line 152 below?
```
@Override
public long nullValueCount() {
return nullValueCount;
}
```
##########
core/src/main/java/org/apache/iceberg/StatsUtil.java:
##########
@@ -19,171 +19,291 @@
package org.apache.iceberg;
import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
-import java.util.Collections;
-import java.util.Comparator;
import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Collectors;
-import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import java.util.Map;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
-import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
class StatsUtil {
- private static final Logger LOG = LoggerFactory.getLogger(StatsUtil.class);
- static final Set<Integer> SUPPORTED_METADATA_FIELD_IDS =
- ImmutableSet.of(
- MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId(),
MetadataColumns.ROW_ID.fieldId());
- private static final int FIRST_SUPPORTED_METADATA_FIELD_ID =
- Collections.min(SUPPORTED_METADATA_FIELD_IDS);
- static final int NUM_SUPPORTED_STATS_PER_COLUMN = 200;
- static final int STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS = 9_000;
- static final int STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS = 10_000;
- // exclusive upper bound of the stats field ID range reserved for
content_stats
- static final int STATS_SPACE_FIELD_ID_END = 200_000_000;
- static final int MAX_DATA_STATS_FIELD_ID =
- STATS_SPACE_FIELD_ID_END - NUM_SUPPORTED_STATS_PER_COLUMN;
- // the max data field ID whose stats struct fits within the reserved range
- static final int MAX_DATA_FIELD_ID =
- (MAX_DATA_STATS_FIELD_ID - STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS)
- / NUM_SUPPORTED_STATS_PER_COLUMN;
-
private StatsUtil() {}
- public static int statsFieldIdForField(int fieldId) {
- return SUPPORTED_METADATA_FIELD_IDS.contains(fieldId)
- ? statsFieldIdForReservedField(fieldId)
- : statsFieldIdForDataField(fieldId);
- }
+ private static final int NUM_RESERVED_FIELD_STATS_IDS = 200;
+ private static final int METADATA_STATS_RANGE_START = 9_000;
+ private static final int CONTENT_STATS_RANGE_START = 10_000;
+ private static final int CONTENT_STATS_RANGE_END = 200_000_000; // exclusive
+
+ private static final int DATA_FIELD_ID_START = 0;
+ private static final int DATA_FIELD_ID_END = 999_950; // exclusive
Review Comment:
nit: `999_950` is derived — `(CONTENT_STATS_RANGE_END -
CONTENT_STATS_RANGE_START) / NUM_RESERVED_FIELD_STATS_IDS = (200_000_000 -
10_000) / 200`. A short comment noting the derivation (or expressing it as that
expression directly) would save readers from working it out.
##########
core/src/main/java/org/apache/iceberg/FieldStatsStruct.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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 java.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.ByteBuffers;
+
+class FieldStatsStruct<T> implements FieldStats<T>, StructLike, Serializable {
+ private final Types.StructType struct;
+ private final Type boundType;
+ private final int[] posToOffset;
+ private final int fieldId;
+
+ private Object lowerBound = null;
+ private Object upperBound = null;
+ private boolean tightBounds = false;
+ private Long valueCount = null;
+ private Long nullValueCount = null;
+ private Long nanValueCount = null;
+ private Integer avgValueSize = null;
+
+ FieldStatsStruct(Types.StructType struct) {
+ this.struct = struct;
+ this.posToOffset = posToOffset(struct);
+ this.fieldId = StatsUtil.toFieldId(struct.fields().get(0).fieldId());
+ this.boundType = struct.fieldType("lower_bound");
+ }
+
+ FieldStatsStruct(
+ Types.StructType struct,
+ T lowerBound,
+ T upperBound,
+ boolean tightBounds,
+ long valueCount,
+ long nullValueCount,
+ long nanValueCount,
+ Integer avgValueSize) {
+ this(struct);
+ setLowerBound(lowerBound);
+ setUpperBound(upperBound);
+ this.tightBounds = tightBounds;
+ this.valueCount = valueCount;
+ this.nullValueCount = nullValueCount;
+ this.nanValueCount = nanValueCount;
+ this.avgValueSize = avgValueSize;
+ }
+
+ private FieldStatsStruct(FieldStatsStruct<T> toCopy) {
+ this(toCopy.struct);
+ // bounds are stored using the internal representation, which is a byte
array for binary types
+ this.lowerBound =
+ toCopy.lowerBound instanceof byte[]
+ ? ((byte[]) toCopy.lowerBound).clone()
+ : toCopy.lowerBound;
+ this.upperBound =
+ toCopy.upperBound instanceof byte[]
+ ? ((byte[]) toCopy.upperBound).clone()
+ : toCopy.upperBound;
+ this.tightBounds = toCopy.tightBounds;
+ this.valueCount = toCopy.valueCount;
+ this.nullValueCount = toCopy.nullValueCount;
+ this.nanValueCount = toCopy.nanValueCount;
+ this.avgValueSize = toCopy.avgValueSize;
+ }
+
+ void fromFieldMetrics(FieldMetrics<T> fieldMetrics) {
+ Preconditions.checkArgument(
+ fieldMetrics.id() == fieldId,
+ "Cannot store stats for field ID: %s (expected %s)",
+ fieldMetrics.id(),
+ fieldId);
+
+ setLowerBound(fieldMetrics.lowerBound());
+ setUpperBound(fieldMetrics.upperBound());
+ this.tightBounds = false;
+ this.valueCount = fieldMetrics.valueCount();
+ this.nullValueCount = fieldMetrics.nullValueCount() < 0 ? null :
fieldMetrics.nullValueCount();
+ this.nanValueCount = fieldMetrics.nanValueCount() < 0 ? null :
fieldMetrics.nanValueCount();
+ this.avgValueSize = null;
+ }
+
+ private boolean isBinary() {
+ return boundType != null
+ && (boundType.typeId() == Type.TypeID.FIXED || boundType.typeId() ==
Type.TypeID.BINARY);
+ }
+
+ private void setLowerBound(Object lowerBound) {
+ this.lowerBound = isBinary() ? ByteBuffers.toByteArray((ByteBuffer)
lowerBound) : lowerBound;
+ }
+
+ private void setUpperBound(Object upperBound) {
+ this.upperBound = isBinary() ? ByteBuffers.toByteArray((ByteBuffer)
upperBound) : upperBound;
+ }
+
+ @Override
+ public int fieldId() {
+ return fieldId;
+ }
+
+ @Override
+ public Types.StructType type() {
+ return struct;
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public T lowerBound() {
+ return (T)
+ (lowerBound != null && isBinary() ? ByteBuffer.wrap((byte[])
lowerBound) : lowerBound);
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public T upperBound() {
+ return (T)
+ (upperBound != null && isBinary() ? ByteBuffer.wrap((byte[])
upperBound) : upperBound);
+ }
+
+ @Override
+ public boolean tightBounds() {
+ return tightBounds;
+ }
+
+ @Override
+ public long valueCount() {
+ return valueCount;
+ }
+
+ @Override
+ public long nullValueCount() {
+ return nullValueCount;
+ }
+
+ @Override
+ public long nanValueCount() {
+ return nanValueCount;
+ }
+
+ @Override
+ public Integer avgValueSizeInBytes() {
+ return avgValueSize;
+ }
+
+ @Override
+ public int size() {
+ return struct.fields().size();
+ }
+
+ private Object getOffset(int offset) {
+ return switch (offset) {
+ case StatsUtil.LOWER_BOUND_OFFSET -> lowerBound();
+ case StatsUtil.UPPER_BOUND_OFFSET -> upperBound();
+ case StatsUtil.TIGHT_BOUNDS_OFFSET -> tightBounds;
+ case StatsUtil.VALUE_COUNT_OFFSET -> valueCount;
+ case StatsUtil.NULL_VALUE_COUNT_OFFSET -> nullValueCount;
+ case StatsUtil.NAN_VALUE_COUNT_OFFSET -> nanValueCount;
+ case StatsUtil.AVG_VALUE_SIZE_OFFSET -> avgValueSize;
+ default -> throw new UnsupportedOperationException("Unsupported stats
offset: " + offset);
+ };
+ }
+
+ @Override
+ public <C> C get(int pos, Class<C> javaClass) {
+ return javaClass.cast(getOffset(posToOffset[pos]));
+ }
+
+ private void setOffset(int offset, Object value) {
+ switch (offset) {
+ case StatsUtil.LOWER_BOUND_OFFSET -> setLowerBound(value);
+ case StatsUtil.UPPER_BOUND_OFFSET -> setUpperBound(value);
+ case StatsUtil.TIGHT_BOUNDS_OFFSET -> this.tightBounds = (Boolean) value;
+ case StatsUtil.VALUE_COUNT_OFFSET -> this.valueCount = (Long) value;
+ case StatsUtil.NULL_VALUE_COUNT_OFFSET -> this.nullValueCount = (Long)
value;
Review Comment:
the `FieldStats` interface changed the getter to return primitive type. not
sure what's the reason there. With that, we need to check and prevent null
object value here?
##########
core/src/main/java/org/apache/iceberg/FieldStatsStruct.java:
##########
@@ -0,0 +1,220 @@
+/*
+ * 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 java.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.types.Type;
+import org.apache.iceberg.types.Types;
+import org.apache.iceberg.util.ByteBuffers;
+
+class FieldStatsStruct<T> implements FieldStats<T>, StructLike, Serializable {
+ private final Types.StructType struct;
+ private final Type boundType;
+ private final int[] posToOffset;
+ private final int fieldId;
+
+ private Object lowerBound = null;
+ private Object upperBound = null;
+ private boolean tightBounds = false;
+ private Long valueCount = null;
+ private Long nullValueCount = null;
+ private Long nanValueCount = null;
+ private Integer avgValueSize = null;
+
+ FieldStatsStruct(Types.StructType struct) {
+ this.struct = struct;
+ this.posToOffset = posToOffset(struct);
+ this.fieldId = StatsUtil.toFieldId(struct.fields().get(0).fieldId());
+ this.boundType = struct.fieldType("lower_bound");
+ }
+
+ FieldStatsStruct(
+ Types.StructType struct,
+ T lowerBound,
+ T upperBound,
+ boolean tightBounds,
+ long valueCount,
+ long nullValueCount,
+ long nanValueCount,
+ Integer avgValueSize) {
+ this(struct);
+ setLowerBound(lowerBound);
+ setUpperBound(upperBound);
+ this.tightBounds = tightBounds;
+ this.valueCount = valueCount;
+ this.nullValueCount = nullValueCount;
+ this.nanValueCount = nanValueCount;
+ this.avgValueSize = avgValueSize;
+ }
+
+ private FieldStatsStruct(FieldStatsStruct<T> toCopy) {
+ this(toCopy.struct);
+ // bounds are stored using the internal representation, which is a byte
array for binary types
+ this.lowerBound =
+ toCopy.lowerBound instanceof byte[]
Review Comment:
I thought the setter wraps `byte[]` to `ByteBuffer`. should we check
ByteBuffer here?
```
private void setLowerBound(Object lowerBound) {
this.lowerBound = isBinary() ? ByteBuffers.toByteArray((ByteBuffer)
lowerBound) : lowerBound;
}
```
##########
core/src/test/java/org/apache/iceberg/TestContentStatsStruct.java:
##########
@@ -0,0 +1,261 @@
+/*
+ * 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.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.util.Comparator;
+import java.util.List;
+import org.apache.iceberg.TestHelpers.RoundTripSerializer;
+import org.apache.iceberg.inmemory.InMemoryOutputFile;
+import org.apache.iceberg.io.CloseableIterable;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
+import org.apache.iceberg.types.Comparators;
+import org.apache.iceberg.types.TypeUtil;
+import org.apache.iceberg.types.Types;
+import org.junit.jupiter.api.Named;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.FieldSource;
+import org.mockito.Mockito;
+
+public class TestContentStatsStruct {
+ private static final Schema SCHEMA =
+ new Schema(
+ required(1, "id", Types.LongType.get()),
+ optional(2, "data", Types.StringType.get()),
+ optional(3, "score", Types.DoubleType.get()));
+
+ private static final Types.StructType CONTENT_STATS_STRUCT =
+ StatsUtil.statsReadSchema(SCHEMA, ImmutableList.of(1, 2, 3));
+ private static final Types.StructType UNKNOWN_FIELD_STATS_STRUCT =
+ StatsUtil.fieldStatsStruct(false, Types.IntegerType.get(), 10_800,
MetricsModes.Full.get());
+
+ private static final FieldStats<Long> ID_STATS =
+ new FieldStatsStruct<>(
+ CONTENT_STATS_STRUCT.field("id").type().asStructType(), 0L, 25L,
true, 26L, 0L, 0L, null);
+ private static final FieldStats<String> DATA_STATS =
+ new FieldStatsStruct<>(
+ CONTENT_STATS_STRUCT.field("data").type().asStructType(),
+ "a",
+ "z",
+ true,
+ 26L,
+ 0L,
+ 0L,
+ null);
+
+ @Test
+ public void testEmptyContentStats() {
+ ContentStats stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+
+ assertThat(stats.statsFor(1)).isNull();
+ assertThat(stats.statsFor(2)).isNull();
+ assertThat(stats.statsFor(3)).isNull();
+ assertThat(stats.statsFor(4)).as("Should ignore unknown field
IDs").isNull();
+ }
+
+ @Test
+ public void testSetStats() {
+ ContentStatsStruct stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+
+ stats.setStats(1, ID_STATS);
+ stats.setStats(2, DATA_STATS);
+
+ assertThat(stats.statsFor(1)).isEqualTo(ID_STATS);
+ assertThat(stats.statsFor(2)).isEqualTo(DATA_STATS);
+ assertThat(stats.statsFor(3)).isNull();
+ }
+
+ @Test
+ public void testSetStatsWrongId() {
+ ContentStatsStruct stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+
+ assertThatThrownBy(() -> stats.setStats(2, ID_STATS))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Mismatched field stats for ID 2: actual ID 1");
+ }
+
+ @Test
+ public void testSetStatsUnknownField() {
+ ContentStatsStruct stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+
+ FieldStats<Integer> fieldStats =
+ new FieldStatsStruct<>(UNKNOWN_FIELD_STATS_STRUCT, 0, 10, false, 8, 3,
0, null);
+
+ assertThatThrownBy(() -> stats.setStats(4, fieldStats))
+ .hasMessage("Cannot set stats for unknown field ID: 4")
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ public void testGetByPosition() {
+ // the content stats struct with a known field order
+ Types.StructType contentStatsStruct =
+ Types.StructType.of(
+ CONTENT_STATS_STRUCT.field("data"),
+ CONTENT_STATS_STRUCT.field("score"),
+ CONTENT_STATS_STRUCT.field("id"));
+
+ ContentStatsStruct stats = new ContentStatsStruct(contentStatsStruct);
+
+ stats.setStats(1, ID_STATS);
+ stats.setStats(2, DATA_STATS);
+
+ assertThat(stats.get(0, FieldStats.class)).isEqualTo(DATA_STATS);
+ assertThat(stats.get(1, FieldStats.class)).isNull();
+ assertThat(stats.get(2, FieldStats.class)).isEqualTo(ID_STATS);
+ }
+
+ @Test
+ public void testSetByPosition() {
+ // the content stats struct with a known field order
+ Types.StructType statsStruct =
+ Types.StructType.of(
+ CONTENT_STATS_STRUCT.field("data"),
+ CONTENT_STATS_STRUCT.field("score"),
+ CONTENT_STATS_STRUCT.field("id"));
+
+ ContentStatsStruct stats = new ContentStatsStruct(statsStruct);
+
+ stats.set(1, null);
+ stats.set(2, ID_STATS);
+ stats.set(0, DATA_STATS);
+
+
assertThat(stats.statsFor(SCHEMA.findField("id").fieldId())).isEqualTo(ID_STATS);
+
assertThat(stats.statsFor(SCHEMA.findField("data").fieldId())).isEqualTo(DATA_STATS);
+ assertThat(stats.statsFor(SCHEMA.findField("score").fieldId())).isNull();
+ }
+
+ @Test
+ public void testSize() {
+ ContentStatsStruct stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+
+ assertThat(stats.size()).isEqualTo(3);
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testCopy() {
+ FieldStats<Long> idStats = Mockito.mock(FieldStats.class);
+ FieldStats<Long> idStatsCopy = Mockito.mock(FieldStats.class);
+ Mockito.when(idStats.fieldId()).thenReturn(1);
+ Mockito.when(idStats.copy()).thenReturn(idStatsCopy);
+
+ FieldStats<String> dataStats = Mockito.mock(FieldStats.class);
+ FieldStats<String> dataStatsCopy = Mockito.mock(FieldStats.class);
+ Mockito.when(dataStats.fieldId()).thenReturn(2);
+ Mockito.when(dataStats.copy()).thenReturn(dataStatsCopy);
+
+ ContentStatsStruct stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+ stats.setStats(1, idStats);
+ stats.setStats(2, dataStats);
+
+ ContentStats copy = stats.copy();
+
+ assertThat(copy).isInstanceOf(ContentStatsStruct.class).isNotSameAs(stats);
+ assertThat(copy.type()).isEqualTo(stats.type());
+
+ // each field stats is deep-copied using its own copy() method
+ assertThat(copy.statsFor(1)).isSameAs(idStatsCopy);
+ assertThat(copy.statsFor(2)).isSameAs(dataStatsCopy);
+ assertThat(copy.statsFor(3)).isNull();
+ }
+
+ @Test
+ @SuppressWarnings("unchecked")
+ public void testFilteredCopy() {
+ FieldStats<Long> idStats = Mockito.mock(FieldStats.class);
+ FieldStats<Long> idStatsCopy = Mockito.mock(FieldStats.class);
+ Mockito.when(idStats.fieldId()).thenReturn(1);
+ Mockito.when(idStats.copy()).thenReturn(idStatsCopy);
+
+ FieldStats<String> dataStats = Mockito.mock(FieldStats.class);
+ FieldStats<String> dataStatsCopy = Mockito.mock(FieldStats.class);
+ Mockito.when(dataStats.fieldId()).thenReturn(2);
+ Mockito.when(dataStats.copy()).thenReturn(dataStatsCopy);
+
+ ContentStatsStruct stats = new ContentStatsStruct(CONTENT_STATS_STRUCT);
+ stats.setStats(1, idStats);
+ stats.setStats(2, dataStats);
+
+ // copy just the stats for field ID 2
+ ContentStats copy = stats.copy(ImmutableSet.of(2));
+
+ assertThat(copy).isInstanceOf(ContentStatsStruct.class).isNotSameAs(stats);
+ assertThat(copy.type()).isEqualTo(TypeUtil.select(stats.type(),
ImmutableSet.of(1)));
+
Review Comment:
This assertion seems wrong.
`stats.type()`'s field IDs are stats base IDs (`fieldId * 200 + 10_000`, so
10200 / 10400 / 10600 for this schema), not table field IDs — so both
`Set.of(1)` here and `Set.of(2)` on line 206 filter to an empty struct via
`TypeUtil.select`. Empty equals empty, so the check passes regardless of what
shape `copy.type()` actually has.
##########
core/src/main/java/org/apache/iceberg/StatsUtil.java:
##########
@@ -19,171 +19,291 @@
package org.apache.iceberg;
import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
-import java.util.Collections;
-import java.util.Comparator;
import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Collectors;
-import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import java.util.Map;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
-import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
class StatsUtil {
- private static final Logger LOG = LoggerFactory.getLogger(StatsUtil.class);
- static final Set<Integer> SUPPORTED_METADATA_FIELD_IDS =
- ImmutableSet.of(
- MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId(),
MetadataColumns.ROW_ID.fieldId());
- private static final int FIRST_SUPPORTED_METADATA_FIELD_ID =
- Collections.min(SUPPORTED_METADATA_FIELD_IDS);
- static final int NUM_SUPPORTED_STATS_PER_COLUMN = 200;
- static final int STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS = 9_000;
- static final int STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS = 10_000;
- // exclusive upper bound of the stats field ID range reserved for
content_stats
- static final int STATS_SPACE_FIELD_ID_END = 200_000_000;
- static final int MAX_DATA_STATS_FIELD_ID =
- STATS_SPACE_FIELD_ID_END - NUM_SUPPORTED_STATS_PER_COLUMN;
- // the max data field ID whose stats struct fits within the reserved range
- static final int MAX_DATA_FIELD_ID =
- (MAX_DATA_STATS_FIELD_ID - STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS)
- / NUM_SUPPORTED_STATS_PER_COLUMN;
-
private StatsUtil() {}
- public static int statsFieldIdForField(int fieldId) {
- return SUPPORTED_METADATA_FIELD_IDS.contains(fieldId)
- ? statsFieldIdForReservedField(fieldId)
- : statsFieldIdForDataField(fieldId);
- }
+ private static final int NUM_RESERVED_FIELD_STATS_IDS = 200;
+ private static final int METADATA_STATS_RANGE_START = 9_000;
+ private static final int CONTENT_STATS_RANGE_START = 10_000;
+ private static final int CONTENT_STATS_RANGE_END = 200_000_000; // exclusive
+
+ private static final int DATA_FIELD_ID_START = 0;
+ private static final int DATA_FIELD_ID_END = 999_950; // exclusive
+
+ private static final int LAST_UPDATED_SEQ_NUM_BASE_ID = 9_000;
+ private static final int ROW_ID_BASE_ID = 9_200;
+
+ // Offsets used for individual stats columns
+ static final int LOWER_BOUND_OFFSET = 1;
+ static final int UPPER_BOUND_OFFSET = 2;
+ static final int TIGHT_BOUNDS_OFFSET = 3;
+ static final int VALUE_COUNT_OFFSET = 4;
+ static final int NULL_VALUE_COUNT_OFFSET = 5;
+ static final int NAN_VALUE_COUNT_OFFSET = 6;
+ static final int AVG_VALUE_SIZE_OFFSET = 7;
+
+ // Offsets used within geo_lower struct
+ private static final int GEO_LOWER_X_OFFSET = 10;
+ private static final int GEO_LOWER_Y_OFFSET = 11;
+ private static final int GEO_LOWER_Z_OFFSET = 12;
+ private static final int GEO_LOWER_M_OFFSET = 13;
+
+ // Offsets used within geo_upper struct
+ private static final int GEO_UPPER_X_OFFSET = 14;
+ private static final int GEO_UPPER_Y_OFFSET = 15;
+ private static final int GEO_UPPER_Z_OFFSET = 16;
+ private static final int GEO_UPPER_M_OFFSET = 17;
- private static int statsFieldIdForDataField(int fieldId) {
- if (fieldId < 0 || fieldId > MAX_DATA_FIELD_ID) {
- return -1;
+ private static final Map<Integer, Integer> METADATA_ID_TO_BASE_ID =
+ ImmutableMap.of(
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId(),
LAST_UPDATED_SEQ_NUM_BASE_ID,
+ MetadataColumns.ROW_ID.fieldId(), ROW_ID_BASE_ID);
+
+ /**
+ * Return the base ID of the stats struct for the given field ID, or -1 if
the ID is out of range.
+ *
+ * @param fieldId a table field ID
+ * @return the base ID for a field stats struct, or -1 if stats cannot be
stored
+ */
+ @VisibleForTesting
+ static int toBaseId(int fieldId) {
+ if (fieldId >= DATA_FIELD_ID_START && fieldId < DATA_FIELD_ID_END) {
+ return (fieldId * NUM_RESERVED_FIELD_STATS_IDS) +
CONTENT_STATS_RANGE_START;
}
- return STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS +
(NUM_SUPPORTED_STATS_PER_COLUMN * fieldId);
+ return METADATA_ID_TO_BASE_ID.getOrDefault(fieldId, -1);
}
- private static int statsFieldIdForReservedField(int fieldId) {
- return STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS
- + (NUM_SUPPORTED_STATS_PER_COLUMN * (fieldId -
FIRST_SUPPORTED_METADATA_FIELD_ID));
- }
+ /**
+ * Return the field ID corresponding to the stats field ID.
+ *
+ * @param statId the field ID of a field stats struct or field within a
stats struct
+ * @return ID of the corresponding table field
+ * @throws IllegalArgumentException if the stats ID is not valid
+ * @throws UnsupportedOperationException if the stats ID is for an
unsupported metadata field
+ */
+ static int toFieldId(int statId) {
+ Preconditions.checkArgument(isValidStatId(statId), "Invalid stats field
ID: %s", statId);
- public static int fieldIdForStatsField(int statsFieldId) {
- if (statsFieldId < STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS
- || statsFieldId >= STATS_SPACE_FIELD_ID_END
- || statsFieldId % NUM_SUPPORTED_STATS_PER_COLUMN != 0) {
- return -1;
+ if (statId < CONTENT_STATS_RANGE_START) {
+ if (inBaseIdRange(LAST_UPDATED_SEQ_NUM_BASE_ID, statId)) {
+ return MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId();
+ } else if (inBaseIdRange(ROW_ID_BASE_ID, statId)) {
+ return MetadataColumns.ROW_ID.fieldId();
+ } else {
+ throw new UnsupportedOperationException("Unsupported metadata stats
field ID: " + statId);
+ }
}
- return statsFieldId < STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS
- ? fieldIdForStatsFieldFromReservedField(statsFieldId)
- : fieldIdForStatsFieldFromDataField(statsFieldId);
+ return (statId - CONTENT_STATS_RANGE_START) / NUM_RESERVED_FIELD_STATS_IDS;
}
- private static int fieldIdForStatsFieldFromDataField(int statsFieldId) {
- return (statsFieldId - STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS)
- / NUM_SUPPORTED_STATS_PER_COLUMN;
+ /**
+ * Return the stats offset of a stats field ID.
+ *
+ * @param statId the field ID of a field stats struct or field within a
stats struct
+ * @return offset that identifies the stored metric, or 0 for a stats
struct's ID
+ * @throws IllegalArgumentException if the stats ID is not valid
+ * @throws UnsupportedOperationException if the stats ID is for an
unsupported metadata field
+ */
+ static int statOffset(int statId) {
+ Preconditions.checkArgument(isValidStatId(statId), "Invalid stats field
ID: %s", statId);
+
+ return statId % 200;
Review Comment:
nit: use `NUM_RESERVED_FIELD_STATS_IDS` instead of the literal `200`
##########
core/src/main/java/org/apache/iceberg/StatsUtil.java:
##########
@@ -19,171 +19,291 @@
package org.apache.iceberg;
import static org.apache.iceberg.types.Types.NestedField.optional;
+import static org.apache.iceberg.types.Types.NestedField.required;
-import java.util.Collections;
-import java.util.Comparator;
import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Collectors;
-import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+import java.util.Map;
+import
org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
-import org.apache.iceberg.relocated.com.google.common.collect.Sets;
+import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.TypeUtil;
import org.apache.iceberg.types.Types;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
class StatsUtil {
- private static final Logger LOG = LoggerFactory.getLogger(StatsUtil.class);
- static final Set<Integer> SUPPORTED_METADATA_FIELD_IDS =
- ImmutableSet.of(
- MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId(),
MetadataColumns.ROW_ID.fieldId());
- private static final int FIRST_SUPPORTED_METADATA_FIELD_ID =
- Collections.min(SUPPORTED_METADATA_FIELD_IDS);
- static final int NUM_SUPPORTED_STATS_PER_COLUMN = 200;
- static final int STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS = 9_000;
- static final int STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS = 10_000;
- // exclusive upper bound of the stats field ID range reserved for
content_stats
- static final int STATS_SPACE_FIELD_ID_END = 200_000_000;
- static final int MAX_DATA_STATS_FIELD_ID =
- STATS_SPACE_FIELD_ID_END - NUM_SUPPORTED_STATS_PER_COLUMN;
- // the max data field ID whose stats struct fits within the reserved range
- static final int MAX_DATA_FIELD_ID =
- (MAX_DATA_STATS_FIELD_ID - STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS)
- / NUM_SUPPORTED_STATS_PER_COLUMN;
-
private StatsUtil() {}
- public static int statsFieldIdForField(int fieldId) {
- return SUPPORTED_METADATA_FIELD_IDS.contains(fieldId)
- ? statsFieldIdForReservedField(fieldId)
- : statsFieldIdForDataField(fieldId);
- }
+ private static final int NUM_RESERVED_FIELD_STATS_IDS = 200;
+ private static final int METADATA_STATS_RANGE_START = 9_000;
+ private static final int CONTENT_STATS_RANGE_START = 10_000;
+ private static final int CONTENT_STATS_RANGE_END = 200_000_000; // exclusive
+
+ private static final int DATA_FIELD_ID_START = 0;
+ private static final int DATA_FIELD_ID_END = 999_950; // exclusive
+
+ private static final int LAST_UPDATED_SEQ_NUM_BASE_ID = 9_000;
+ private static final int ROW_ID_BASE_ID = 9_200;
+
+ // Offsets used for individual stats columns
+ static final int LOWER_BOUND_OFFSET = 1;
+ static final int UPPER_BOUND_OFFSET = 2;
+ static final int TIGHT_BOUNDS_OFFSET = 3;
+ static final int VALUE_COUNT_OFFSET = 4;
+ static final int NULL_VALUE_COUNT_OFFSET = 5;
+ static final int NAN_VALUE_COUNT_OFFSET = 6;
+ static final int AVG_VALUE_SIZE_OFFSET = 7;
+
+ // Offsets used within geo_lower struct
+ private static final int GEO_LOWER_X_OFFSET = 10;
+ private static final int GEO_LOWER_Y_OFFSET = 11;
+ private static final int GEO_LOWER_Z_OFFSET = 12;
+ private static final int GEO_LOWER_M_OFFSET = 13;
+
+ // Offsets used within geo_upper struct
+ private static final int GEO_UPPER_X_OFFSET = 14;
+ private static final int GEO_UPPER_Y_OFFSET = 15;
+ private static final int GEO_UPPER_Z_OFFSET = 16;
+ private static final int GEO_UPPER_M_OFFSET = 17;
- private static int statsFieldIdForDataField(int fieldId) {
- if (fieldId < 0 || fieldId > MAX_DATA_FIELD_ID) {
- return -1;
+ private static final Map<Integer, Integer> METADATA_ID_TO_BASE_ID =
+ ImmutableMap.of(
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId(),
LAST_UPDATED_SEQ_NUM_BASE_ID,
+ MetadataColumns.ROW_ID.fieldId(), ROW_ID_BASE_ID);
+
+ /**
+ * Return the base ID of the stats struct for the given field ID, or -1 if
the ID is out of range.
+ *
+ * @param fieldId a table field ID
+ * @return the base ID for a field stats struct, or -1 if stats cannot be
stored
+ */
+ @VisibleForTesting
+ static int toBaseId(int fieldId) {
+ if (fieldId >= DATA_FIELD_ID_START && fieldId < DATA_FIELD_ID_END) {
+ return (fieldId * NUM_RESERVED_FIELD_STATS_IDS) +
CONTENT_STATS_RANGE_START;
}
- return STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS +
(NUM_SUPPORTED_STATS_PER_COLUMN * fieldId);
+ return METADATA_ID_TO_BASE_ID.getOrDefault(fieldId, -1);
}
- private static int statsFieldIdForReservedField(int fieldId) {
- return STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS
- + (NUM_SUPPORTED_STATS_PER_COLUMN * (fieldId -
FIRST_SUPPORTED_METADATA_FIELD_ID));
- }
+ /**
+ * Return the field ID corresponding to the stats field ID.
+ *
+ * @param statId the field ID of a field stats struct or field within a
stats struct
+ * @return ID of the corresponding table field
+ * @throws IllegalArgumentException if the stats ID is not valid
+ * @throws UnsupportedOperationException if the stats ID is for an
unsupported metadata field
+ */
+ static int toFieldId(int statId) {
+ Preconditions.checkArgument(isValidStatId(statId), "Invalid stats field
ID: %s", statId);
- public static int fieldIdForStatsField(int statsFieldId) {
- if (statsFieldId < STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS
- || statsFieldId >= STATS_SPACE_FIELD_ID_END
- || statsFieldId % NUM_SUPPORTED_STATS_PER_COLUMN != 0) {
- return -1;
+ if (statId < CONTENT_STATS_RANGE_START) {
+ if (inBaseIdRange(LAST_UPDATED_SEQ_NUM_BASE_ID, statId)) {
+ return MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId();
+ } else if (inBaseIdRange(ROW_ID_BASE_ID, statId)) {
+ return MetadataColumns.ROW_ID.fieldId();
+ } else {
+ throw new UnsupportedOperationException("Unsupported metadata stats
field ID: " + statId);
+ }
}
- return statsFieldId < STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS
- ? fieldIdForStatsFieldFromReservedField(statsFieldId)
- : fieldIdForStatsFieldFromDataField(statsFieldId);
+ return (statId - CONTENT_STATS_RANGE_START) / NUM_RESERVED_FIELD_STATS_IDS;
}
- private static int fieldIdForStatsFieldFromDataField(int statsFieldId) {
- return (statsFieldId - STATS_SPACE_FIELD_ID_START_FOR_DATA_FIELDS)
- / NUM_SUPPORTED_STATS_PER_COLUMN;
+ /**
+ * Return the stats offset of a stats field ID.
+ *
+ * @param statId the field ID of a field stats struct or field within a
stats struct
+ * @return offset that identifies the stored metric, or 0 for a stats
struct's ID
+ * @throws IllegalArgumentException if the stats ID is not valid
+ * @throws UnsupportedOperationException if the stats ID is for an
unsupported metadata field
+ */
+ static int statOffset(int statId) {
+ Preconditions.checkArgument(isValidStatId(statId), "Invalid stats field
ID: %s", statId);
+
+ return statId % 200;
}
- private static int fieldIdForStatsFieldFromReservedField(int statsFieldId) {
- int fieldId =
- (statsFieldId - STATS_SPACE_FIELD_ID_START_FOR_METADATA_FIELDS)
- / NUM_SUPPORTED_STATS_PER_COLUMN
- + FIRST_SUPPORTED_METADATA_FIELD_ID;
- return SUPPORTED_METADATA_FIELD_IDS.contains(fieldId) ? fieldId : -1;
+ public static Types.NestedField contentStatsField(Types.StructType
contentStats) {
+ return optional(146, "content_stats", contentStats);
}
- public static Types.NestedField contentStatsFor(Schema schema) {
- ContentStatsSchemaVisitor visitor = new ContentStatsSchemaVisitor(schema);
- Types.NestedField result = TypeUtil.visit(schema, visitor);
- if (!visitor.skippedFieldIds.isEmpty()) {
- LOG.warn("Could not create stats schema for field ids: {}",
visitor.skippedFieldIds);
+ public static Types.StructType statsWriteSchema(Schema tableSchema,
MetricsConfig metricsConfig) {
+ Map<Integer, String> idToStatsName =
TypeUtil.indexStatsNames(tableSchema.asStruct());
+ List<Types.NestedField> fieldStructs = Lists.newArrayList();
+
+ for (int id : metricsConfig.metricsFieldIds()) {
+ String fieldName = idToStatsName.get(id);
+ Types.NestedField field = tableSchema.findField(id);
+
+ int baseId = toBaseId(id);
+ Types.StructType fieldStruct =
+ fieldStatsStruct(field.isOptional(), field.type(), baseId,
metricsConfig.columnMode(id));
+
+ if (fieldStruct != null) {
+ fieldStructs.add(optional(baseId, fieldName, fieldStruct));
+ }
}
- return result;
+ return Types.StructType.of(fieldStructs);
}
- private static class ContentStatsSchemaVisitor extends
TypeUtil.SchemaVisitor<Types.NestedField> {
- private final Schema tableSchema;
- private final List<Types.NestedField> statsFields = Lists.newArrayList();
- private final Set<Integer> skippedFieldIds = Sets.newLinkedHashSet();
+ /**
+ * Produce a schema to read content stats for the given table field IDs.
+ *
+ * @param tableSchema a schema
+ * @param fieldIds an iterable of field IDs to project stats for
+ * @return a content stats struct for a read
+ */
+ public static Types.StructType statsReadSchema(Schema tableSchema,
Iterable<Integer> fieldIds) {
+ Map<Integer, String> idToStatsName =
TypeUtil.indexStatsNames(tableSchema.asStruct());
+ List<Types.NestedField> fieldStructs = Lists.newArrayList();
- ContentStatsSchemaVisitor(Schema tableSchema) {
- this.tableSchema = tableSchema;
- }
+ for (int id : fieldIds) {
+ String fieldName = idToStatsName.get(id);
+ Types.NestedField field = tableSchema.findField(id);
Review Comment:
`findField(id)` returns null for unknown IDs, so `field.isOptional()` on the
next line NPEs. `statsReadSchema` is public with caller-supplied IDs .
Skip-on-null or `Preconditions.checkArgument` with the ID in the message would
be safer.
Line 137 in `statsWriteSchema` has the same problem but sources IDs from
`metricsConfig`, so less exposed.
--
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]
