raghavyadav01 commented on code in PR #18643:
URL: https://github.com/apache/pinot/pull/18643#discussion_r3501889490
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/IndexLoadingConfig.java:
##########
@@ -408,6 +414,47 @@ private <K, V> Map<K, V> unmodifiable(Map<K, V> map) {
return map == null ? null : Collections.unmodifiableMap(map);
}
+ // NOTE: entries added here are written directly into
_indexConfigsByColName. Any later call that
+ // sets _dirty = true (e.g. setSegmentTier, addKnownColumns) will trigger
refreshIndexConfigs()
+ // and rebuild the map from scratch, wiping these entries. Callers must
ensure no dirty-flag
+ // mutations happen between this call and the point where the configs are
consumed.
+ public void addOpenStructChildConfigs(SegmentMetadataImpl segmentMetadata) {
+ if (_indexConfigsByColName == null || _dirty) {
+ refreshIndexConfigs();
+ }
+ for (Map.Entry<String, ColumnMetadata> entry :
segmentMetadata.getColumnMetadataMap().entrySet()) {
+ String childColumn = entry.getKey();
+ if (!childColumn.contains(OpenStructNaming.SEPARATOR) ||
_indexConfigsByColName.containsKey(childColumn)) {
+ continue;
+ }
+ if (OpenStructNaming.isSparseColumn(childColumn)) {
+ continue;
+ }
+ String parentColumn = OpenStructNaming.parseParentColumn(childColumn);
+ FieldIndexConfigs parentConfigs =
_indexConfigsByColName.get(parentColumn);
+ if (parentConfigs == null) {
+ continue;
Review Comment:
`addOpenStructChildConfigs` writes entries into `_indexConfigsByColName`
that a later `_dirty = true` (`setSegmentTier`, `addKnownColumns`) would
rebuild away — the comment flags this but doesn't prevent it. Could these child
configs live in a separate field that `refreshIndexConfigs()` re-merges, so
correctness doesn't depend on caller ordering?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1310,6 +1334,20 @@ public void commit() {
}
}
+ /**
+ * Returns the per-column mutable OPEN_STRUCT index, or {@code null} if the
column is not OPEN_STRUCT
+ * or the index has not been initialized.
+ */
+ @Nullable
+ public MutableOpenStructIndex getOpenStructIndex(String column) {
+ IndexContainer container = _indexContainerMap.get(column);
+ if (container == null) {
+ return null;
Review Comment:
This new public method returns the concrete `MutableOpenStructIndex` rather
than the `OpenStructIndexReader` SPI. Is the concrete type needed by callers,
or could it return the interface to keep the impl encapsulated?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -135,6 +147,34 @@ public void indexColumn(String columnName, @Nullable int[]
sortedDocIds, IndexSe
}
Review Comment:
OPEN_STRUCT special-casing via `instanceof OpenStructDataSource` now lives
in 4 separate pipeline classes (here, `PinotSegmentRecordReader`,
`RealtimeSegmentStatsContainer`, `ImmutableSegmentImpl`). It mirrors the
existing MAP handling so it's consistent, but is there appetite to push this
behind a polymorphic hook on the DataSource/IndexType so future composite types
don't each add another `instanceof` fork?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java:
##########
@@ -0,0 +1,265 @@
+/**
+ * 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.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.index.datasource.BaseDataSource;
+import
org.apache.pinot.segment.local.segment.index.datasource.ImmutableDataSource;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.datasource.DataSource;
+import org.apache.pinot.segment.spi.datasource.DataSourceMetadata;
+import org.apache.pinot.segment.spi.datasource.OpenStructDataSource;
+import org.apache.pinot.segment.spi.index.column.ColumnIndexContainer;
+import org.apache.pinot.segment.spi.index.reader.Dictionary;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext;
+import org.apache.pinot.segment.spi.index.reader.NullValueVectorReader;
+import org.apache.pinot.segment.spi.partition.PartitionFunction;
+import org.apache.pinot.spi.data.ComplexFieldSpec;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+
+/// Per-key {@link DataSource} accessor for sealed (immutable) segments with
an OPEN_STRUCT column.
+///
+/// Always columnar — there is no blob branch. Every key that was dense enough
during segment
+/// creation gets its own materialized {@link DataSource} (forward index +
optional inverted index /
+/// dictionary). Keys that did not meet the density threshold are stored in an
optional sparse
+/// column; the sparse {@link DataSource} is returned for any unmaterialized
key lookup.
+///
+/// Use [#isMaterialized(String)] and [#isFullyMaterialized()] together to
choose
+/// the query execution path:
+/// - Materialized key → fast path via per-key DataSource (inverted/dictionary
index available).
+/// - Not materialized + not fully materialized → fall back to the sparse
DataSource.
+/// - Not materialized + fully materialized → key is definitively absent;
short-circuit.
+///
+/// Thread-safety: immutable after construction; safe for concurrent reads.
+public class ImmutableOpenStructDataSource extends BaseDataSource implements
OpenStructDataSource {
+ private final ComplexFieldSpec _fieldSpec;
+ private final Map<String, DataSource> _perKeyDataSources;
+ @Nullable
+ private final DataSource _sparseDataSource;
+
+ public ImmutableOpenStructDataSource(ComplexFieldSpec fieldSpec, Map<String,
DataSource> perKeyDataSources,
+ @Nullable DataSource sparseDataSource, DataSourceMetadata
dataSourceMetadata,
+ ColumnIndexContainer indexContainer) {
+ super(dataSourceMetadata, indexContainer);
+ _fieldSpec = fieldSpec;
+ _perKeyDataSources = perKeyDataSources;
+ _sparseDataSource = sparseDataSource;
+ }
+
+ /// Convenience constructor for segment-load time. Synthesizes a minimal
{@link DataSourceMetadata}
+ /// for the parent OPEN_STRUCT column (which has no on-disk presence of its
own) and uses an empty
+ /// {@link ColumnIndexContainer} — all real readers live on the per-key data
sources.
+ public ImmutableOpenStructDataSource(ComplexFieldSpec fieldSpec, Map<String,
DataSource> perKeyDataSources,
+ @Nullable DataSource sparseDataSource, int numDocs) {
+ this(fieldSpec, perKeyDataSources, sparseDataSource,
+ new ImmutableOpenStructDataSourceMetadata(fieldSpec, numDocs),
+ new ColumnIndexContainer.FromMap.Builder().build());
+ }
+
+ @Override
+ public ComplexFieldSpec getFieldSpec() {
+ return _fieldSpec;
+ }
+
+ @Override
+ @Nullable
+ public DataSource getDataSource(String key) {
+ DataSource ds = _perKeyDataSources.get(key);
+ return ds != null ? ds : _sparseDataSource;
+ }
+
+ @Override
+ public boolean isMaterialized(String key) {
+ return _perKeyDataSources.containsKey(key);
+ }
+
+ @Override
+ public boolean isFullyMaterialized() {
+ return _sparseDataSource == null;
+ }
+
+ @Override
+ public Map<String, DataSource> getDataSources() {
+ return _perKeyDataSources;
+ }
+
+ @Override
+ @Nullable
+ public DataSourceMetadata getDataSourceMetadata(String key) {
+ DataSource ds = _perKeyDataSources.get(key);
+ return ds != null ? ds.getDataSourceMetadata() : null;
+ }
+
+ @Override
+ @Nullable
+ public ColumnIndexContainer getIndexContainer(String key) {
+ DataSource ds = _perKeyDataSources.get(key);
+ return ds instanceof ImmutableDataSource immutableDs ?
immutableDs.getIndexContainer() : null;
+ }
+
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ @Override
+ @Nullable
+ public Map<String, Object> getMapValue(int docId) {
+ Map<String, Object> result = null;
+
+ for (Map.Entry<String, DataSource> entry : _perKeyDataSources.entrySet()) {
+ Object value = readValue(entry.getValue(), docId);
+ if (value != null) {
+ if (result == null) {
+ result = new HashMap<>();
+ }
+ result.put(entry.getKey(), value);
+ }
+ }
+
+ if (_sparseDataSource != null) {
+ Object sparseValue = readValue(_sparseDataSource, docId);
+ if (sparseValue instanceof String) {
+ String json = (String) sparseValue;
+ if (!json.isEmpty()) {
+ try {
+ Map<String, Object> sparseMap = JsonUtils.stringToObject(json,
Map.class);
Review Comment:
Sparse keys are parsed via `JsonUtils.stringToObject(json, Map.class)`,
yielding generic JSON types. When `PinotSegmentRecordReader` rebuilds this map
and feeds it into a new segment build (minion merge/rollup), sparse values come
back as inferred JSON types rather than their original declared types — so a
key could re-infer differently (e.g. LONG→INT) across rebuilds. Is round-trip
type stability a concern here, or are sparse keys untyped by design?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java:
##########
@@ -0,0 +1,265 @@
+/**
+ * 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.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.index.datasource.BaseDataSource;
+import
org.apache.pinot.segment.local.segment.index.datasource.ImmutableDataSource;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.datasource.DataSource;
+import org.apache.pinot.segment.spi.datasource.DataSourceMetadata;
+import org.apache.pinot.segment.spi.datasource.OpenStructDataSource;
+import org.apache.pinot.segment.spi.index.column.ColumnIndexContainer;
+import org.apache.pinot.segment.spi.index.reader.Dictionary;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext;
+import org.apache.pinot.segment.spi.index.reader.NullValueVectorReader;
+import org.apache.pinot.segment.spi.partition.PartitionFunction;
+import org.apache.pinot.spi.data.ComplexFieldSpec;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+
+/// Per-key {@link DataSource} accessor for sealed (immutable) segments with
an OPEN_STRUCT column.
+///
+/// Always columnar — there is no blob branch. Every key that was dense enough
during segment
+/// creation gets its own materialized {@link DataSource} (forward index +
optional inverted index /
+/// dictionary). Keys that did not meet the density threshold are stored in an
optional sparse
+/// column; the sparse {@link DataSource} is returned for any unmaterialized
key lookup.
+///
+/// Use [#isMaterialized(String)] and [#isFullyMaterialized()] together to
choose
+/// the query execution path:
+/// - Materialized key → fast path via per-key DataSource (inverted/dictionary
index available).
+/// - Not materialized + not fully materialized → fall back to the sparse
DataSource.
+/// - Not materialized + fully materialized → key is definitively absent;
short-circuit.
+///
+/// Thread-safety: immutable after construction; safe for concurrent reads.
+public class ImmutableOpenStructDataSource extends BaseDataSource implements
OpenStructDataSource {
+ private final ComplexFieldSpec _fieldSpec;
+ private final Map<String, DataSource> _perKeyDataSources;
+ @Nullable
+ private final DataSource _sparseDataSource;
+
+ public ImmutableOpenStructDataSource(ComplexFieldSpec fieldSpec, Map<String,
DataSource> perKeyDataSources,
+ @Nullable DataSource sparseDataSource, DataSourceMetadata
dataSourceMetadata,
+ ColumnIndexContainer indexContainer) {
+ super(dataSourceMetadata, indexContainer);
+ _fieldSpec = fieldSpec;
+ _perKeyDataSources = perKeyDataSources;
+ _sparseDataSource = sparseDataSource;
+ }
+
+ /// Convenience constructor for segment-load time. Synthesizes a minimal
{@link DataSourceMetadata}
+ /// for the parent OPEN_STRUCT column (which has no on-disk presence of its
own) and uses an empty
+ /// {@link ColumnIndexContainer} — all real readers live on the per-key data
sources.
+ public ImmutableOpenStructDataSource(ComplexFieldSpec fieldSpec, Map<String,
DataSource> perKeyDataSources,
+ @Nullable DataSource sparseDataSource, int numDocs) {
+ this(fieldSpec, perKeyDataSources, sparseDataSource,
+ new ImmutableOpenStructDataSourceMetadata(fieldSpec, numDocs),
+ new ColumnIndexContainer.FromMap.Builder().build());
+ }
+
Review Comment:
This extends `BaseDataSource` but backs the parent with an empty
`ColumnIndexContainer`, so `getForwardIndex()`/`getDictionary()` return null on
the parent — which `BaseDataSource` consumers generally assume is non-null. Is
`BaseDataSource` the right base here vs. a thinner `DataSource` impl, given the
parent has no physical column of its own? Any column-wide sweep calling
`getForwardIndex()` could NPE on the parent.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -1708,6 +1749,11 @@ private class IndexContainer implements Closeable {
}
DataSource toDataSource() {
+ if (_fieldSpec.getDataType() == DataType.OPEN_STRUCT) {
Review Comment:
`toDataSource()` routes any OPEN_STRUCT column to
`MutableOpenStructDataSource` with `(MutableOpenStructIndex) idx`, but the
index is only created when the config is present + enabled. If OPEN_STRUCT is
declared with the index disabled, `idx` is null → NPE on query/seal. Guard for
null, or enforce "OPEN_STRUCT requires the index enabled"?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/BaseSegmentCreator.java:
##########
@@ -552,6 +553,31 @@ protected void writeMetadata()
hasDictionary, dictionaryElementSize, fwdConfig.getEncodingType(),
false);
}
+ // OPEN_STRUCT splitters produce per-key materialized child columns
(col$key, col$__sparse__) that
+ // are not in _columnStatisticsMap. Merge their pre-built metadata into
the segment properties so
+ // each child appears as its own column at load time, and register them as
dimensions so the V3
+ // converter and SegmentMetadataImpl discover their index files.
+ List<String> openStructChildColumns = new ArrayList<>();
+ for (ColumnIndexCreators columnIndexCreators : _colIndexes.values()) {
+ for (IndexCreator indexCreator : columnIndexCreators.getIndexCreators())
{
+ if (indexCreator instanceof ColumnarOpenStructIndexCreator) {
+ ColumnarOpenStructIndexCreator splitter =
(ColumnarOpenStructIndexCreator) indexCreator;
+ for (Map.Entry<String, PropertiesConfiguration> childEntry
+ : splitter.getMaterializedColumnMetadata().entrySet()) {
+ String childCol = childEntry.getKey();
+ PropertiesConfiguration childProps = childEntry.getValue();
+ childProps.getKeys().forEachRemaining(key ->
properties.setProperty(key, childProps.getProperty(key)));
+ openStructChildColumns.add(childCol);
+ }
+ }
+ }
+ }
+ if (!openStructChildColumns.isEmpty()) {
+ List<String> dimensions = new ArrayList<>(_config.getDimensions());
+ dimensions.addAll(openStructChildColumns);
+ properties.setProperty(DIMENSIONS, dimensions);
Review Comment:
The splitter's synthetic children (`col$key`, `col$__sparse__`) are appended
to the segment `DIMENSIONS` property. Any consumer that reads the segment
dimension list and reconciles it against the table schema will now see columns
that don't exist in the schema — could this confuse metadata/validation paths?
Is `DIMENSIONS` the right place vs. a dedicated child-column marker in metadata?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java:
##########
@@ -0,0 +1,265 @@
+/**
+ * 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.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.segment.local.segment.index.datasource.BaseDataSource;
+import
org.apache.pinot.segment.local.segment.index.datasource.ImmutableDataSource;
+import org.apache.pinot.segment.spi.Constants;
+import org.apache.pinot.segment.spi.datasource.DataSource;
+import org.apache.pinot.segment.spi.datasource.DataSourceMetadata;
+import org.apache.pinot.segment.spi.datasource.OpenStructDataSource;
+import org.apache.pinot.segment.spi.index.column.ColumnIndexContainer;
+import org.apache.pinot.segment.spi.index.reader.Dictionary;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReader;
+import org.apache.pinot.segment.spi.index.reader.ForwardIndexReaderContext;
+import org.apache.pinot.segment.spi.index.reader.NullValueVectorReader;
+import org.apache.pinot.segment.spi.partition.PartitionFunction;
+import org.apache.pinot.spi.data.ComplexFieldSpec;
+import org.apache.pinot.spi.data.FieldSpec;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+
+/// Per-key {@link DataSource} accessor for sealed (immutable) segments with
an OPEN_STRUCT column.
+///
+/// Always columnar — there is no blob branch. Every key that was dense enough
during segment
+/// creation gets its own materialized {@link DataSource} (forward index +
optional inverted index /
+/// dictionary). Keys that did not meet the density threshold are stored in an
optional sparse
+/// column; the sparse {@link DataSource} is returned for any unmaterialized
key lookup.
+///
+/// Use [#isMaterialized(String)] and [#isFullyMaterialized()] together to
choose
+/// the query execution path:
+/// - Materialized key → fast path via per-key DataSource (inverted/dictionary
index available).
+/// - Not materialized + not fully materialized → fall back to the sparse
DataSource.
+/// - Not materialized + fully materialized → key is definitively absent;
short-circuit.
+///
+/// Thread-safety: immutable after construction; safe for concurrent reads.
+public class ImmutableOpenStructDataSource extends BaseDataSource implements
OpenStructDataSource {
+ private final ComplexFieldSpec _fieldSpec;
+ private final Map<String, DataSource> _perKeyDataSources;
+ @Nullable
+ private final DataSource _sparseDataSource;
+
+ public ImmutableOpenStructDataSource(ComplexFieldSpec fieldSpec, Map<String,
DataSource> perKeyDataSources,
+ @Nullable DataSource sparseDataSource, DataSourceMetadata
dataSourceMetadata,
+ ColumnIndexContainer indexContainer) {
+ super(dataSourceMetadata, indexContainer);
+ _fieldSpec = fieldSpec;
+ _perKeyDataSources = perKeyDataSources;
+ _sparseDataSource = sparseDataSource;
+ }
+
+ /// Convenience constructor for segment-load time. Synthesizes a minimal
{@link DataSourceMetadata}
+ /// for the parent OPEN_STRUCT column (which has no on-disk presence of its
own) and uses an empty
+ /// {@link ColumnIndexContainer} — all real readers live on the per-key data
sources.
+ public ImmutableOpenStructDataSource(ComplexFieldSpec fieldSpec, Map<String,
DataSource> perKeyDataSources,
+ @Nullable DataSource sparseDataSource, int numDocs) {
+ this(fieldSpec, perKeyDataSources, sparseDataSource,
+ new ImmutableOpenStructDataSourceMetadata(fieldSpec, numDocs),
+ new ColumnIndexContainer.FromMap.Builder().build());
+ }
+
+ @Override
+ public ComplexFieldSpec getFieldSpec() {
+ return _fieldSpec;
+ }
+
+ @Override
+ @Nullable
+ public DataSource getDataSource(String key) {
+ DataSource ds = _perKeyDataSources.get(key);
+ return ds != null ? ds : _sparseDataSource;
+ }
+
+ @Override
+ public boolean isMaterialized(String key) {
+ return _perKeyDataSources.containsKey(key);
+ }
+
+ @Override
+ public boolean isFullyMaterialized() {
+ return _sparseDataSource == null;
+ }
+
+ @Override
+ public Map<String, DataSource> getDataSources() {
+ return _perKeyDataSources;
+ }
+
+ @Override
+ @Nullable
+ public DataSourceMetadata getDataSourceMetadata(String key) {
Review Comment:
`getDataSources()` returns only `_perKeyDataSources` (dense), so
sparse-backed keys are invisible to callers iterating this map, while
`getDataSource(key)` *does* fall back to sparse. Is that asymmetry intended? If
so, worth documenting the contract as "materialized keys only" so callers don't
treat it as the full key set.
--
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]
