anuragrai16 commented on code in PR #18710: URL: https://github.com/apache/pinot/pull/18710#discussion_r3382039897
########## pinot-spi/src/main/java/org/apache/pinot/spi/data/OpenStructTypeInference.java: ########## @@ -0,0 +1,68 @@ +/** + * 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.pinot.spi.data; + +import javax.annotation.Nullable; +import org.apache.pinot.spi.utils.PinotDataType; + + +/// Infers the {@link FieldSpec.DataType} for an OPEN_STRUCT key from raw ingested values when the key has +/// no declared child {@link FieldSpec}. This is OPEN_STRUCT-specific policy (it keeps TIMESTAMP, folds +/// DATE/TIME/UUID to STRING, widens BYTE/CHARACTER/SHORT to INT, and returns {@code null} for values that +/// cannot be represented as a stored column type), distinct from the JSON-node-based inference in +/// {@code JsonUtils.valueOf}. +public final class OpenStructTypeInference { + private OpenStructTypeInference() { + } + + /// Infers the {@link FieldSpec.DataType} from a raw ingested value. Returns {@code null} when the value + /// cannot be represented as a stored column type; callers decide whether to drop the entry or fall back Review Comment: >callers decide whether to drop the entry or fall back to a default (e.g. STRING). What config controls this ? ########## pinot-spi/src/main/java/org/apache/pinot/spi/data/OpenStructNaming.java: ########## @@ -36,4 +36,41 @@ public static String materializedColumnName(String openStructColumn, String key) public static String sparseColumnName(String openStructColumn) { return openStructColumn + SEPARATOR + SPARSE_SUFFIX; } + + /// Returns true if the given column name is a materialized OPEN_STRUCT child column + /// (dense materialized key or the sparse JSON column). + public static boolean isMaterializedOpenStructColumn(String columnName) { + return columnName.indexOf(SEPARATOR.charAt(0)) > 0; + } + + /// Returns true if the given column name is the sparse JSON column for some + /// OPEN_STRUCT parent. + public static boolean isSparseColumn(String columnName) { + int sep = columnName.indexOf(SEPARATOR.charAt(0)); Review Comment: What are the callers of `isMaterializedOpenStructColumn` `isSparseColumn` - Is there no better metadata that can be used to track this to remove the reliance on SEPARATOR ? What is the impact of change in separator in the future version, and how do we prevent it from changing (tests etc.) ? Are there other datatypes in Pinot that rely on this ? ########## pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/datasource/OpenStructDataSource.java: ########## @@ -61,4 +62,13 @@ public interface OpenStructDataSource extends DataSource { /// Returns the ColumnIndexContainer for the given key's values. ColumnIndexContainer getIndexContainer(String key); + + /// Reconstructs the full OPEN_STRUCT value for {@code docId} as a {@code Map<String, Object>}, or + /// {@code null} when no key is present at that doc. Used by the realtime seal path to re-feed the + /// OPEN_STRUCT column into the immutable segment build. + @Nullable + default Map<String, Object> getMapValue(int docId) { + throw new UnsupportedOperationException( Review Comment: Next PR ? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java: ########## @@ -1620,6 +1604,23 @@ private static void validateIndexingConfigAndFieldConfigList(TableConfig tableCo validateMultiColumnTextIndex(indexingConfig.getMultiColumnTextIndexConfig()); + // OPEN_STRUCT materialized child columns use a reserved separator '$' in their name. When any Review Comment: Does this mean any existing schema that may have the '$' in their column name cannot add this data type ? ########## pinot-spi/src/main/java/org/apache/pinot/spi/data/OpenStructTypeInference.java: ########## @@ -0,0 +1,68 @@ +/** + * 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.pinot.spi.data; + +import javax.annotation.Nullable; +import org.apache.pinot.spi.utils.PinotDataType; + + +/// Infers the {@link FieldSpec.DataType} for an OPEN_STRUCT key from raw ingested values when the key has +/// no declared child {@link FieldSpec}. This is OPEN_STRUCT-specific policy (it keeps TIMESTAMP, folds +/// DATE/TIME/UUID to STRING, widens BYTE/CHARACTER/SHORT to INT, and returns {@code null} for values that +/// cannot be represented as a stored column type), distinct from the JSON-node-based inference in +/// {@code JsonUtils.valueOf}. +public final class OpenStructTypeInference { + private OpenStructTypeInference() { + } + + /// Infers the {@link FieldSpec.DataType} from a raw ingested value. Returns {@code null} when the value + /// cannot be represented as a stored column type; callers decide whether to drop the entry or fall back + /// to a default (e.g. STRING). + @Nullable + public static FieldSpec.DataType inferDataType(Object rawValue) { + switch (PinotDataType.getSingleValueType(rawValue)) { + case INTEGER: + case BYTE: + case CHARACTER: + case SHORT: + return FieldSpec.DataType.INT; + case LONG: + return FieldSpec.DataType.LONG; + case FLOAT: + return FieldSpec.DataType.FLOAT; + case DOUBLE: + return FieldSpec.DataType.DOUBLE; + case BIG_DECIMAL: + return FieldSpec.DataType.BIG_DECIMAL; + case BOOLEAN: + return FieldSpec.DataType.BOOLEAN; + case TIMESTAMP: + return FieldSpec.DataType.TIMESTAMP; + case STRING: + case DATE: + case TIME: Review Comment: TIME to String, or should it be TIMESTAMP ? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructSupportedIndexes.java: ########## @@ -0,0 +1,38 @@ +/** + * 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.pinot.segment.local.segment.index.openstruct; + +import java.util.Set; +import org.apache.pinot.segment.spi.index.StandardIndexes; + + +/// The set of index types (by pretty name) supported on OPEN_STRUCT materialized child columns. A key's +/// `FieldConfig` may declare only these; non-vetted indexes are rejected at table-config validation. +/// `dictionary` is built structurally (lifecycle CUSTOM); `forward` is always written. +public final class OpenStructSupportedIndexes { + private OpenStructSupportedIndexes() { + } + + public static final Set<String> ALLOWED_PRETTY_NAMES = Set.of( + StandardIndexes.forward().getPrettyName(), + StandardIndexes.dictionary().getPrettyName(), Review Comment: Do we need explicit dictionary here when the RAW v/s default already controls this ? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructIndexType.java: ########## @@ -0,0 +1,183 @@ +/** + * 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.pinot.segment.local.segment.index.openstruct; + +import com.fasterxml.jackson.databind.JsonNode; +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import javax.annotation.Nullable; +import org.apache.pinot.segment.spi.ColumnMetadata; +import org.apache.pinot.segment.spi.creator.IndexCreationContext; +import org.apache.pinot.segment.spi.index.AbstractIndexType; +import org.apache.pinot.segment.spi.index.ColumnConfigDeserializer; +import org.apache.pinot.segment.spi.index.FieldIndexConfigs; +import org.apache.pinot.segment.spi.index.IndexConfigDeserializer; +import org.apache.pinot.segment.spi.index.IndexHandler; +import org.apache.pinot.segment.spi.index.IndexReaderFactory; +import org.apache.pinot.segment.spi.index.StandardIndexes; +import org.apache.pinot.segment.spi.index.creator.ColumnarOpenStructIndexCreator; +import org.apache.pinot.segment.spi.index.mutable.MutableIndex; +import org.apache.pinot.segment.spi.index.mutable.provider.MutableIndexContext; +import org.apache.pinot.segment.spi.index.reader.OpenStructIndexReader; +import org.apache.pinot.segment.spi.store.SegmentDirectory; +import org.apache.pinot.spi.config.table.FieldConfig; +import org.apache.pinot.spi.config.table.OpenStructIndexConfig; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.data.FieldSpec; +import org.apache.pinot.spi.data.Schema; + + +/// Index type for the OPEN_STRUCT index on OPEN_STRUCT columns. +/// +/// The OPEN_STRUCT index has no reader of its own — per-key materialized columns are loaded by +/// the standard `PhysicalColumnIndexContainer` and served via standard index readers. This +/// type exists for SPI registration, config deserialization, and validation. +public class OpenStructIndexType + extends AbstractIndexType<OpenStructIndexConfig, OpenStructIndexReader, ColumnarOpenStructIndexCreator> { + + public static final String INDEX_DISPLAY_NAME = "open_struct"; + private static final List<String> EXTENSIONS = Collections.singletonList(".open_struct.idx"); + + protected OpenStructIndexType() { + super(StandardIndexes.OPEN_STRUCT_ID); + } + + @Override + public Class<OpenStructIndexConfig> getIndexConfigClass() { + return OpenStructIndexConfig.class; + } + + @Override + public OpenStructIndexConfig getDefaultConfig() { + return OpenStructIndexConfig.DEFAULT; + } + + @Override + public void validate(FieldIndexConfigs indexConfigs, FieldSpec fieldSpec, TableConfig tableConfig) { + // The default OpenStructIndexConfig is auto-applied to every column; only enforce on OPEN_STRUCT + // fields. Non-OPEN_STRUCT columns cannot meaningfully opt in to this index. + if (fieldSpec.getDataType() != FieldSpec.DataType.OPEN_STRUCT) { + return; Review Comment: Should this be an IllegalArgumentException instead ? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/stats/AbstractColumnStatisticsCollector.java: ########## @@ -56,13 +56,28 @@ public abstract class AbstractColumnStatisticsCollector implements ColumnStatist protected Comparable _previousValue = null; public AbstractColumnStatisticsCollector(String column, StatsCollectorConfig statsCollectorConfig) { - _fieldSpec = statsCollectorConfig.getFieldSpecForColumn(column); - Preconditions.checkArgument(_fieldSpec != null, "Failed to find column: %s", column); - _storedType = _fieldSpec.getDataType().getStoredType(); - _sorted = _fieldSpec.isSingleValueField(); - _fieldConfig = statsCollectorConfig.getFieldConfigForColumn(column); - _partitionFunction = statsCollectorConfig.getPartitionFunction(column); - _partitions = _partitionFunction != null ? new HashSet<>() : null; + this(getRequiredFieldSpec(column, statsCollectorConfig), + statsCollectorConfig.getFieldConfigForColumn(column), + statsCollectorConfig.getPartitionFunction(column)); + } + + /// Constructs a collector directly from a [FieldSpec], without a [StatsCollectorConfig]. Lets callers + /// that operate outside schema-driven segment generation (e.g. OPEN_STRUCT materialized child columns, + /// whose synthetic columns exist in no schema) reuse the standard stats collectors. + public AbstractColumnStatisticsCollector(FieldSpec fieldSpec, @Nullable FieldConfig fieldConfig, + @Nullable PartitionFunction partitionFunction) { + _fieldSpec = fieldSpec; + _storedType = fieldSpec.getDataType().getStoredType(); + _sorted = fieldSpec.isSingleValueField(); + _fieldConfig = fieldConfig; + _partitionFunction = partitionFunction; + _partitions = partitionFunction != null ? new HashSet<>() : null; + } + + private static FieldSpec getRequiredFieldSpec(String column, StatsCollectorConfig statsCollectorConfig) { Review Comment: Is this needed or can be inlined ? ########## pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/OpenStructSupportedIndexes.java: ########## @@ -0,0 +1,38 @@ +/** + * 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.pinot.segment.local.segment.index.openstruct; + +import java.util.Set; +import org.apache.pinot.segment.spi.index.StandardIndexes; + + +/// The set of index types (by pretty name) supported on OPEN_STRUCT materialized child columns. A key's +/// `FieldConfig` may declare only these; non-vetted indexes are rejected at table-config validation. +/// `dictionary` is built structurally (lifecycle CUSTOM); `forward` is always written. +public final class OpenStructSupportedIndexes { Review Comment: Does it need to be a new class, can it be appended to an existing Constant class or the `OpenStructIndexType` ? -- 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]
