anoopj commented on code in PR #17159: URL: https://github.com/apache/iceberg/pull/17159#discussion_r3591825068
########## 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); Review Comment: Do something like Iterables.transform(fieldIds, StatsUtil::toBaseId)) to get to the right ID space? ########## 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) { Review Comment: I _think_ this is mixing up two ID spaces. `fieldIds` are table field IDs, which makes sense because we are relying on it to build `idToFieldStats`. But the `toCopy.struct` is keyed off the stats field IDs. As a result, I think `TypeUtil.select(toCopy.struct, fieldIds)` might be returning an empty struct. ########## 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()); Review Comment: Won't this cause a NPE if the requested ID has no stored stats? Should we skip the IDs rather than dereference them? ########## 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()) { Review Comment: The field order is non-determinisitc because we are iterating over a hash map. Should we sort by IDs? There is no correctness issue here. ########## 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: Yes, I think there is a bug in the code, so the test is passing vacuously. I left a comment in ContentsStatsStruct. -- 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]
