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: [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]
