You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tarun11Mavani (via GitHub)" <gi...@apache.org> on 2026/09/25 13:15:00 UTC
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
tarun11Mavani commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4102762211
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
Review Comment:
a multi-value key cannot be sealed.
getValue() calls the single-value _forwardIndex.getDictId(docId, null) with no shape branch. For an MV key _forwardIndex is the FixedByteMVMutableForwardIndex allocated at L114, which implements only getDictIdMV (L253, L267). So the call resolves through MutableForwardIndex.getDictId(int, ctx) → getDictId(int), whose default is throw new UnsupportedOperationException().
Let's handle this path and also add a test to pin it.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
@@ -168,6 +191,26 @@ public void setValue(int docId, Object value) {
_lastIndexedDocId = docId;
}
+ /// Indexes a list of values at `docId`. Elements must already be coerced to the stored type. Throws
+ /// [IllegalArgumentException] when the list is longer than [#MAX_NUM_MULTI_VALUES]; the caller drops and meters
+ /// it, the same as a value that cannot be coerced.
+ public void setValues(int docId, Object[] values) {
+ int[] dictIds = new int[values.length];
+ for (int i = 0; i < values.length; i++) {
+ dictIds[i] = _dictionary.index(values[i]);
Review Comment:
setValues mutates the dictionary before the length check runs, so a rejected row leaves the dictionary ahead of the inverted index permanently.
Can add a check here for `values.length <= MAX_NUM_MULTI_VALUES`
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableOpenStructIndex.java:
##########
@@ -294,7 +373,9 @@ public ColumnMetadata getColumnMetadata(String key) {
}
FieldSpec spec = _childFieldSpecs.get(key);
if (spec == null) {
- spec = new DimensionFieldSpec(key, col.getStoredType(), true);
+ // Shape comes from the column, not a fixed single-value assumption: a key holding lists has a multi-value
+ // forward index, and metadata that disagreed with it would tell the query planner the wrong thing.
+ spec = new DimensionFieldSpec(key, col.getStoredType(), col.isSingleValue());
}
return new SimpleColumnMetadata(spec, _capacity);
Review Comment:
**Blocking: a range predicate on a multi-value key throws `NegativeArraySizeException`.**
`SimpleColumnMetadata.getMaxNumberOfMultiValues()` returns `UNAVAILABLE` (`-1`). For an MV key that flows through `ImmutableDataSourceMetadata` L53-57 into `MVScanDocIdIterator` L56, whose matchers allocate `new int[_maxNumValuesPerMVEntry]` in field initializers (L172, L184, …) — so `new int[-1]`, thrown from the constructor.
Reachable because of the fix on L378: the shape is now honest (correct), so `ScanBasedFilterOperator` L60-65 takes the MV branch for the first time. The `-1` was always wrong, just unreachable. Applies to declared and undeclared MV keys alike.
Unconditional for range: `getIndexes` L361-364 gives a consuming key only forward/dictionary/inverted — no sorted or range index — so per `FilterOperatorUtils` L116-126 a `RANGE` predicate always falls to the scan. `col['scores'] > 100` throws. `EQ`/`IN` are fine, served by the inverted index.
**Fix — `SimpleColumnMetadata`**: Add an overloaded ctor, defaulting to `UNAVAILABLE` so the MAP caller is unaffected:
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -196,62 +212,133 @@ public Set<String> classify() {
private void addMap(@Nullable Map<String, Object> map) {
if (map != null && !map.isEmpty()) {
- for (Map.Entry<String, Object> entry : map.entrySet()) {
- String key = entry.getKey();
- Object rawValue = entry.getValue();
- if (rawValue == null) {
- continue;
- }
- if (_config.isIgnoredKey(key)) {
- _ignoredKeyDropCount++;
- continue;
- }
- FieldSpec keySpec = _childFieldSpecs.get(key);
- DataType valueType;
- if (keySpec != null) {
- valueType = keySpec.getDataType();
+ OpenStructKeyFlattener.flatten(map, _maxNestedKeyDepth, this::addEntry);
+ }
+ _numDocs++;
+ }
+
+ /// Accumulates one flat key of the current document. `container` marks a key whose value is a nested object
+ /// rendered as JSON text; see [#classify()] for why those are held out of automatic dense selection.
+ private void addEntry(String key, @Nullable Object rawValue, boolean container) {
+ if (rawValue == null) {
+ return;
+ }
+ if (_config.isIgnoredKey(key)) {
+ _ignoredKeyDropCount++;
+ return;
+ }
+ if (container) {
+ _containerKeys.add(key);
+ }
+ FieldSpec keySpec = _childFieldSpecs.get(key);
+ // Shape is decided by the first value the key presents and then sticks, exactly as its type does. A
+ // collection arriving on a key whose shape is already scalar is handled as any other value it cannot
+ // represent -- stringified on a STRING key, a coercion failure on a typed one -- rather than reshaping a
+ // column other documents already wrote to.
+ boolean multiValueKey;
+ if (_presenceBitmaps.containsKey(key)) {
+ multiValueKey = _multiValueKeys.contains(key);
+ } else {
+ // A declaration decides the shape in both directions -- a key declared single-value stays single-value
+ // even when its values are collections, because the declaration is what the user asked for. Only an
+ // undeclared key takes its shape from the data.
+ multiValueKey = keySpec != null
+ ? !keySpec.isSingleValueField()
+ : OpenStructTypeInference.asMultiValue(rawValue) != null;
+ if (multiValueKey) {
+ _multiValueKeys.add(key);
Review Comment:
**An undeclared key whose first value is an empty list ends up multi-value sealed and single-value consuming.**
`_multiValueKeys.add(key)` runs before the empty-list return just below it, so the emptiness is discarded but the shape decision is kept. `writeDenseKeyColumn` L500 then reads `singleValue = !_multiValueKeys.contains(key)`, and L507 wraps the stored scalars into one-element arrays. `MutableOpenStructIndex.indexEntry` L136 returns before recording anything, so on that side the next value decides the shape.
Reproduced with docs `{"tags":[]}`, then `{"tags":"x"}`, then docs carrying only `host`:
```
SHAPE consuming singleValue=true sealed singleValue=false
```
The comment on the mutable side says it matches the splitter, which is the intent, but the splitter's `_multiValueKeys.add` sits above its early return and the mutable path has no equivalent side table.
Worth checking the splitter on its own terms as well. Because the empty list never populates `_presenceBitmaps`, doc 1 re-enters the else branch at L240 and is stored as a scalar; only the `replaceAll` at L507 rescues it into multi-value shape at write time. The per-document decision and the final column shape disagree until that fixup runs.
**Fix:** record the shape only alongside a stored value, on both paths, or give the mutable index the same sticky side table the splitter has. I am able to reproduce this with a UT.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java:
##########
@@ -106,7 +115,33 @@ public DataSource getDataSource(String key) {
return new NullDataSource(getValueFieldSpec(key), getDataSourceMetadata().getNumDocs());
}
return _sparseKeyDataSourceCache.computeIfAbsent(key,
- k -> new SparseKeyDataSource(getValueFieldSpec(k), _sparseBlobReader));
+ k -> new SparseKeyDataSource(getValueFieldSpec(k), _sparseBlobReader, maxNumValues(k)));
+ }
+
+ /// Field spec for a key's values, with an undeclared sparse key's shape taken from the segment's sparse
+ /// multi-value manifest. Which tier a key lands on is a tuning decision, so it must not decide the key's
+ /// shape: without this, the same rows would report `STRING[]` on a segment that materialized the key and a
+ /// scalar `STRING` holding `["a","b"]` on one that put it in the blob, and a query fanning out over both
+ /// would see two shapes for one column.
+ @Override
+ public FieldSpec getValueFieldSpec(String key) {
+ FieldSpec childFieldSpec = _fieldSpec.getChildFieldSpec(key);
+ if (childFieldSpec != null) {
+ return childFieldSpec;
+ }
+ boolean singleValue = _sparseMultiValueKeys == null || !_sparseMultiValueKeys.containsKey(key);
+ return new DimensionFieldSpec(key, FieldSpec.DataType.STRING, singleValue);
Review Comment:
**Question: is the type asymmetry between tiers intended for now?**
Same undeclared key holding `[1, 2, 3]`, same rows, built twice: once pinned dense, once with `maxDenseKeys=0` forcing it into the blob.
```
dense materialized=true singleValue=false INT
sparse materialized=false singleValue=false STRING
```
Shape agrees, so `sparseMultiValueKeys` is doing its job. Type does not: `writeDenseKeyColumn` L493 uses the inferred element type, while this line hardcodes STRING.
The STRING fallback predates this PR, since the old `OpenStructDataSource` L53-56 default did the same, so the line itself is not a regression. What is new is the disagreement. Before fix 5 an undeclared list was a STRING scalar on both tiers, so they matched.
Given this method's javadoc says "Which tier a key lands on is a tuning decision, so it must not decide the key's shape", is type deliberately left for a follow-up, or worth persisting alongside the shape in the same manifest?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/ProjectionBlock.java:
##########
@@ -61,15 +70,99 @@ public BlockValSet getBlockValueSet(ExpressionContext expression) {
public BlockValSet getBlockValueSet(String column) {
DataSource dataSource = _dataSourceMap.get(column);
// An OPEN_STRUCT parent is only a handle for per-key resolution — it has no forward index, so DataFetcher does
- // not register it and it cannot be read as a column. Reject it here rather than letting the missing
- // ColumnValueReader surface as an NPE.
- if (dataSource instanceof OpenStructDataSource) {
- throw new BadQueryRequestException(
- "OPEN_STRUCT column: " + column + " cannot be selected directly; use " + column + "['key']");
+ // not register it and it cannot be read through the block cache. Assemble its document here instead, which is
+ // what the storage layer's contract defers to the query layer. Without this `SELECT col` and, worse, `SELECT *`
+ // both failed outright on any table carrying one.
+ if (dataSource instanceof OpenStructDataSource openStructDataSource) {
+ return openStructDocuments(column, openStructDataSource);
}
return new ProjectionBlockValSet(_dataBlockCache, column, dataSource);
}
+ /// The column's whole document per row, as JSON text.
+ ///
+ /// Assembled through [OpenStructDataSource#openMapValueReader()], the reconstruction the storage layer already
+ /// owns and the seal path already uses. Going key by key over [OpenStructDataSource#getDataSources()] instead
+ /// reads only the materialized keys -- sparse keys share one JSON column and have no DataSource of their own --
+ /// so every unmaterialized key would silently vanish from the document.
+ private BlockValSet openStructDocuments(String column, OpenStructDataSource openStructDataSource) {
+ int numDocs = getNumDocs();
+ int[] docIds = getDocIds();
+ String[] documents = new String[numDocs];
+ try (MapValueReader reader = openStructDataSource.openMapValueReader()) {
+ for (int i = 0; i < numDocs; i++) {
+ Map<String, Object> document = reader.getMapValue(docIds[i]);
+ documents[i] = document == null ? "{}" : JsonUtils.objectToString(renderDocument(document));
+ }
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to read OPEN_STRUCT column: " + column, e);
+ }
+ return new OpenStructDocumentBlockValSet(documents);
+ }
+
+ /// The document as it should read back: nested, and with no key spelled twice.
+ ///
+ /// A key nested inside an object is materialized under its path -- `configApi.timeTaken` -- while the object it
+ /// came from stays in the document whole, so the reconstruction carries the same value both ways. The object is
+ /// the shape the source had, so it wins and the paths into it are dropped. `.` is an ordinary key character with
+ /// no escape, and that is exactly what makes the container's own entry the thing that disambiguates: a dotted key
+ /// whose prefix is not itself a key was never a path, so it stays a key spelled with a dot.
+ private static Map<String, Object> renderDocument(Map<String, Object> document) {
+ Map<String, Object> rendered = new LinkedHashMap<>(document.size());
+ for (Map.Entry<String, Object> entry : document.entrySet()) {
+ if (!isPathIntoPresentObject(entry.getKey(), document)) {
+ rendered.put(entry.getKey(), renderValue(entry.getValue()));
+ }
+ }
+ return rendered;
+ }
+
+ /// Whether `key` is a path into an object that the document also carries whole. `configApi.timeTaken` is, when
+ /// `configApi` is a key; a key the document literally spells with a dot is not, because no prefix of it is a key.
+ private static boolean isPathIntoPresentObject(String key, Map<String, Object> document) {
+ int dot = key.indexOf(OpenStructKeyFlattener.PATH_SEPARATOR);
+ while (dot >= 0) {
+ if (document.get(key.substring(0, dot)) instanceof Map) {
Review Comment:
I think this guard can't fire: a container key is never a `Map` here.
A nested object is stored as JSON text. At depth >= 2 the flattener does it outright (`toJson` at `OpenStructKeyFlattener` L148, and `addEntry`'s javadoc says "rendered as JSON text"); at depth 1 `inferDataType` has no `MAP` case, so the value falls back to `STRING` via `MapUtils.toString`. The read path keeps it text deliberately (`SparseKeyDataSource` L172/L328, which is fix #1). So `document.get(prefix)` is a `String`, the guard returns false, and `renderValue` L141 falls through the same way.
At depth >= 2 that means the container renders as an escaped string with every flattened path beside it, the double-render this commit set out to remove. At depth 1 nothing duplicates, but the object still comes back escaped rather than nested. The tests miss it because they stub `openMapValueReader()` with hand-built maps: `testNestedObjectReadsBackNested` builds `configApi` as a literal `LinkedHashMap`, which reconstruction never produces.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/SparseKeyDataSource.java:
##########
@@ -153,6 +187,187 @@ public byte[] getBytes(int docId, ForwardIndexReaderContext context) {
}, FieldSpec.DEFAULT_DIMENSION_NULL_VALUE_OF_BYTES);
Review Comment:
getBytes L187 and getBytesMV L344 fall back to the hardcoded type default, while the other twelve typed getters route theirs through declaredOr(...).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
@@ -243,6 +286,25 @@ public void readDictIds(int[] docIds, int length, int[] dictIdBuffer, ForwardInd
}
}
+ @Override
+ public int getDictIdMV(int docId, int[] dictIdBuffer, ForwardIndexReaderContext context) {
+ if (docId > _lastIndexedDocId) {
Review Comment:
**An absent doc below the watermark reads as empty here; the sealed side reads one default value.**
The guard catches only `docId > _lastIndexedDocId`, so a doc lacking this key while later docs carry it falls through to `FixedByteMVMutableForwardIndex` L253, which reads `length` from a zero-initialized header row and returns 0 values. `writeDenseKeyColumn` writes `new Object[]{childFieldSpec.getDefaultNullValue()}` for that same doc. Ingest `{"tags":["a"]}`, `{}`, `{"tags":["b"]}` and doc 1 reads `[]` consuming, `["null"]` sealed.
The single-value twin at L268 survives only because `FixedByteSVMutableForwardIndex` is a flat array whose unwritten slots read dictId 0, the reserved default, which the constructor comment relies on by name. A header-based index breaks that assumption.
`testConsumingMatchesSealedForMultiValueKey` misses it because `mvDoc` puts `tags` on docs 0 to 4 only, so every absent doc sits past the watermark and takes the guarded branch.
**Fix:** consult `_presenceBitmap`, the way `getValue` L224 already does. An empty list never reaches storage, so length 0 always means absent. Both overloads need it, L290 and L301. Surfaces with null handling off, since both tiers also mark the doc null.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ItemTransformFunction.java:
##########
@@ -121,13 +122,71 @@ public long[] transformToLongValuesSV(ValueBlock valueBlock) {
return valueBlock.getBlockValueSet(_keyPath).getLongValuesSV();
}
+ @Override
+ public float[] transformToFloatValuesSV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getFloatValuesSV();
+ }
+
@Override
public double[] transformToDoubleValuesSV(ValueBlock valueBlock) {
return valueBlock.getBlockValueSet(_keyPath).getDoubleValuesSV();
}
+ /// Without this the base class has no way to produce a BIG_DECIMAL: its conversion switch widens INT, LONG, FLOAT,
+ /// DOUBLE, STRING and BYTES into one, but a key whose own type is already BIG_DECIMAL matches no case and throws
+ /// `Cannot read SV BIG_DECIMAL as BIG_DECIMAL`. Reading it straight off the key's value source is both the fix and
+ /// the cheaper path.
+ @Override
+ public BigDecimal[] transformToBigDecimalValuesSV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getBigDecimalValuesSV();
+ }
+
@Override
public String[] transformToStringValuesSV(ValueBlock valueBlock) {
return valueBlock.getBlockValueSet(_keyPath).getStringValuesSV();
}
+
+ @Override
+ public byte[][] transformToBytesValuesSV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getBytesValuesSV();
+ }
+
+ // A key can hold a list, in which case its value source is multi-value and the engine asks for the values that
+ // way. The result metadata above already reports the key's own shape, so these are the reads that shape implies;
+ // without them a multi-value key would be storable and describable but not readable.
+
+ @Override
+ public int[][] transformToDictIdsMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getDictionaryIdsMV();
+ }
+
+ @Override
+ public int[][] transformToIntValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getIntValuesMV();
+ }
+
+ @Override
+ public long[][] transformToLongValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getLongValuesMV();
+ }
+
+ @Override
+ public float[][] transformToFloatValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getFloatValuesMV();
+ }
+
+ @Override
+ public double[][] transformToDoubleValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getDoubleValuesMV();
+ }
+
+ @Override
+ public String[][] transformToStringValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getStringValuesMV();
+ }
+
+ @Override
+ public byte[][][] transformToBytesValuesMV(ValueBlock valueBlock) {
+ return valueBlock.getBlockValueSet(_keyPath).getBytesValuesMV();
Review Comment:
**`transformToBigDecimalValuesMV` is missing from this block.**
A sparse key is never dictionary-encoded, and `BaseTransformFunction.transformToBigDecimalValuesMV` has no `BIG_DECIMAL` case in its no-dictionary switch, so a declared BIG_DECIMAL multi-value key throws on read:
```
java.lang.IllegalStateException: Cannot read MV BIG_DECIMAL as BIG_DECIMAL
```
Reachable because this PR adds `SparseKeyDataSource.getBigDecimalMV`. The SV override at L140 exists for the same gap on the other side, so this is its twin: `return valueBlock.getBlockValueSet(_keyPath).getBigDecimalValuesMV();`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org