You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "raghavyadav01 (via GitHub)" <gi...@apache.org> on 2026/09/20 21:05:00 UTC
[PR] Materialize an OPEN_STRUCT key holding lists as a multi-value column [pinot]
raghavyadav01 opened a new pull request, #19608:
URL: https://github.com/apache/pinot/pull/19608
Stacked on #19607 (the declared-child-spec fix), which this needs for the child's single/multi-value flag. Review that one first; the second commit here is the multi-value work.
### The problem
A key whose values are collections has nowhere to go. `OpenStructTypeInference` does not map a collection, so the key falls back to STRING and the column stores the list's `toString`:
```
{"tags": [1, 2, 3]} → props$tags = "[1, 2, 3]" // not even JSON
```
Nothing inside it can be filtered, counted or grouped, and the elements' own types are gone.
### Change
Materialize such a key as a **multi-value column of its element type**, written by the standard MV creators. `writeColumnIndexes` was already generic over `IndexCreator`, which has a multi-value `add`, so this needed the shape and the per-element dictIds rather than a new creator.
Shape follows the same rule the type already does, and for the same reason — a column cannot be reshaped underneath documents that already wrote to it:
- **A declaration decides it, both ways.** A key declared single-value stays single-value even when its values are collections, because that is what the user asked for.
- **Otherwise the first value decides, and sticks.** A collection arriving later on a scalar-shaped key is handled like any other value the column cannot represent — stringified on a STRING key, a coercion failure on a typed one.
- **Elements that disagree resolve to STRING**, the same answer a key whose values drift across rows gets, so a mixed array keeps every element rather than losing the odd ones.
Edge cases, all covered by tests:
| case | result |
|---|---|
| scalar on a multi-value key | one element |
| absent document | one default element, as the standard creator writes for an absent MV field |
| empty list | not a value — no elements, no type to infer, no empty state on disk — so the key is absent for that document, exactly as a null value is |
| list longer than the mutable column's maximum | dropped and metered as a coercion failure, rather than failing the segment |
### Both tiers
`MutableKeyColumn` gains a multi-value mode over `FixedByteMVMutableForwardIndex`, so the consuming tier resolves the same values by its own route. `OpenStructConsumingSealedParityTest` covers a multi-value key element for element either side of `seal()`, pinning the values themselves rather than only cross-tier equality.
### Tests
- `OpenStructMultiValueKeyTest` — element type, numeric elements, disagreeing elements, declared SV with list values, declared MV with scalar values, scalar on an MV key, shape stickiness, absent docs, empty lists.
- `OpenStructConsumingSealedParityTest` — new multi-value parity case.
78 OPEN_STRUCT tests across `pinot-spi` and `pinot-segment-local` pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 merged PR #19608:
URL: https://github.com/apache/pinot/pull/19608
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19608:
URL: https://github.com/apache/pinot/pull/19608#issuecomment-5874885505
Documented the user-facing OPEN_STRUCT nested-path, multi-value key, and whole-column projection behavior in merged pinot-docs PR: https://github.com/pinot-contrib/pinot-docs/pull/1072
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
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
Re: [PR] Seven OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, and sparse blob reads [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4060770703
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -196,62 +212,125 @@ 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.
+ Object[] elements = OpenStructTypeInference.asMultiValue(rawValue);
Review Comment:
:nit:
asMultiValue runs for every key of every row and the result is discarded ten lines later when the key is already single-value. Moving it behind that check costs nothing, though the saving is only two instanceof calls.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/OpenStructKeyFlattener.java:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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 com.fasterxml.jackson.core.JsonProcessingException;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+/// Turns a nested OPEN_STRUCT document into flat keys, so a value buried under an object is
+/// addressable as a key of its own.
+///
+/// OPEN_STRUCT keys a document one level deep: `col['device']` names an entry of the top-level
+/// map. A document like `{"device": {"os": "ios"}}` therefore has exactly one key, `device`, whose
+/// value is an object -- there is no key that names the `os` inside it, so no column is ever
+/// materialized for it and no predicate can reach it without decoding the object per row.
+///
+/// Flattening makes the **path** the key: `device.os`. That key is materialized, indexed, filtered
+/// and projected like any other, with no change to the key/value contract -- `.` is an ordinary
+/// character in a key, so `col['device.os']` is already valid syntax.
+///
+/// The container keeps its own entry, serialized as JSON text, so `col['device']` still returns the
+/// whole object after its leaves have been split out.
+///
+/// ```
+/// {"device": {"os": "ios", "ver": 17}} maxDepth = 2
Review Comment:
What should happen when flattening produces a key the document already has?
`.` is an ordinary key character. That's what makes `col['device.os']` valid — and it's also the path separator, so at `maxNestedKeyDepth > 1` one document can emit the same key twice:
```json
{"a.b": 100, "a": {"b": 200}}
```
`flatten` emits `[a.b, a, a.b]` here. Should `a.b` be the literal key, the flattened path, last-one-wins — or is this document outside the contract?
The two tiers answer differently today. Three docs at depth 2 (`{"a.b":100,"a":{"b":200}}`, `{"a.b":300}`, `{"a.b":400}`):
| tier | doc0 | doc1 | doc2 |
|---|---|---|---|
| consuming | 200 | 300 | 400 |
| sealed | 100 | 200 | 300 |
`MutableOpenStructIndex` writes by docId, so the second emission overwrites. `OpenStructColumnSplitter` appends to `_values` while `bitmap.add(_numDocs)` is idempotent, so the list runs one longer than the bitmap's cardinality — and `writeColumnIndexes` pairs them positionally (`values.get(ordinal++)`), shifting every later document. doc2's `400` is never written; no exception.
Only reachable with the feature on, but dotted attribute names alongside nested objects is common in OTel-style data. `OpenStructConsumingSealedParityTest` has a nested-path case but no colliding one, so it passes.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -375,14 +477,20 @@ private void writeDenseKeyColumn(String key)
RoaringBitmap presence = _presenceBitmaps.get(key);
List<Object> values = _values.get(key);
- // TODO: Honor the declared child field spec (field type, single/multi-value and custom default null value) instead
- // of synthesizing a single-value dimension of the stored type, so a document without the key reads the same as
- // through OpenStructDataSource.getValueFieldSpec, which returns the declared spec for a key absent from the
- // segment. See https://github.com/apache/pinot/issues/19466
- // Synthetic field spec for the materialized child. Its natural Pinot dimension null value is the value
- // stored for absent docs, so column metadata stays consistent with on-disk content.
- DimensionFieldSpec childFieldSpec = new DimensionFieldSpec(materializedCol, storedType, true);
- Object defaultValue = childFieldSpec.getDefaultNullValue();
+ boolean singleValue = !_multiValueKeys.contains(key);
Review Comment:
This is the only read of `_multiValueKeys`, so the shape fix applies to the dense tier only. Which tier a key lands on is a tuning decision, not a property of the data.
`writeSparseJsonColumn` never consults it, nothing forces a multi-value key dense, and an undeclared sparse key resolves through `getValueFieldSpec` to `new DimensionFieldSpec(key, STRING, true)`, served by a reader whose `isSingleValue()` is a literal `true` with no MV getters.
Two segments, identical rows (`{"tags": ["a","b"]}` × 4), differing only in `maxDenseKeys`:
```
DENSE materialized=true type=STRING singleValue=false doc0=[a, b]
SPARSE materialized=false type=STRING singleValue=true doc0=["a","b"]
```
One reports `STRING[]` with two elements, the other a scalar `STRING` holding `["a","b"]`. A query fans out over both, so the broker sees two shapes for one column and `col['tags'] = 'a'` matches only the first. A key drifting below `denseKeyMinFillRate` flips shape with no ingestion-time signal.
Is dense-only deliberate for now? The commit reads as though the sparse blob covers the case, which is true of the blob but not of the reader over it.
Could `OpenStructMultiValueKeyTest` get a sparse case? The ten new cases are all dense, which is why this passes. Same rows at `maxDenseKeys = -1` vs `0`, asserting the same shape and the same elements both ways, would pin it.
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4118367542
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableKeyColumn.java:
##########
Review Comment:
Confirmed and fixed in c10fc97 — `getValue` now branches on shape and reads an MV key through `getDictIdMV`. Reachable exactly as you describe, through `getMapValue` → `ProjectionBlock`, so it took out `SELECT` of the column and the seal path.
##########
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:
Confirmed and fixed in c10fc97. `SimpleColumnMetadata` gains an overload carrying the real max-MV length, defaulting to `UNAVAILABLE` so the MAP caller is untouched, and `MutableOpenStructIndex` passes `MutableKeyColumn.MAX_NUM_MULTI_VALUES` for a multi-value key. Your read of why it only surfaced now is right: the shape fix is what first sends these keys down the MV scan branch.
##########
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:
Confirmed and fixed in c10fc97 — the length check now runs before the dictionary is touched.
Worth recording the blast radius for anyone reading later: the orphaned dictIds are unreferenced, so they inflate distinct-value counts and seal-time cardinality rather than mis-mapping any row. Still wrong, just not corrupting.
##########
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:
Confirmed and fixed in c10fc97 — both overloads now consult `_presenceBitmap` instead of only the watermark, exactly as you suggested.
The class comment was the giveaway: it says in-range holes need no guard because chunks are zero-initialized and read as dictId 0. True for the flat single-value array, false for a multi-value index that stores a length header — a hole reads as zero values, i.e. an empty list, which is a value no document held and no sealed segment produces.
##########
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:
Confirmed and fixed in c10fc97 — the shape is now recorded only alongside a stored value, so the empty-list row returns before `_multiValueKeys.add` and both tiers take their shape from the first row that actually stores something.
I went with your first option rather than giving the mutable index a side table, since the splitter returning early is what already makes the two paths agree.
##########
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:
Confirmed and fixed in c10fc97 — both now route through `declaredOr(byte[].class, ...)` like the other twelve.
##########
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:
Confirmed and fixed in c10fc97 — added, with a note on why the base class cannot serve it (its no-dictionary switch widens other types into BIG_DECIMAL but has no case for a key already declared BIG_DECIMAL).
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4118366205
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/OpenStructKeyFlattener.java:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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 com.fasterxml.jackson.core.JsonProcessingException;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+/// Turns a nested OPEN_STRUCT document into flat keys, so a value buried under an object is
+/// addressable as a key of its own.
+///
+/// OPEN_STRUCT keys a document one level deep: `col['device']` names an entry of the top-level
+/// map. A document like `{"device": {"os": "ios"}}` therefore has exactly one key, `device`, whose
+/// value is an object -- there is no key that names the `os` inside it, so no column is ever
+/// materialized for it and no predicate can reach it without decoding the object per row.
+///
+/// Flattening makes the **path** the key: `device.os`. That key is materialized, indexed, filtered
+/// and projected like any other, with no change to the key/value contract -- `.` is an ordinary
+/// character in a key, so `col['device.os']` is already valid syntax.
+///
+/// The container keeps its own entry, serialized as JSON text, so `col['device']` still returns the
+/// whole object after its leaves have been split out.
+///
+/// ```
+/// {"device": {"os": "ios", "ver": 17}} maxDepth = 2
Review Comment:
I could not reproduce the double emission — `a.b` is emitted once, and the literal wins.
`flattenInto` guards every synthesized path with `carriedByEnclosingMap` (L144: `boolean emit = !synthesized || (!carriedByEnclosingMap(level, path) && guard.firstEmission(path));`). Recursing into `a`, the path `a.b` walks back to the root level where `prefix == null`, so `suffix == path` and `containsKey("a.b")` is true (L165-173) — `emit` is false.
So for `{"a.b": 100, "a": {"b": 200}}` the emissions are `[a.b → 100, a → {"b":200}]`. The nested `200` is not addressable as its own key, which is the documented rule: a literal key the document carries wins, and addressability is never allowed to shadow data that was already addressable.
`OpenStructKeyFlattenerTest` pins this in both orderings and asserts an emission *count*, not just the value — L140-143 and L150-153 (`assertEquals(_emissions.get("a.b"), (Integer) 1, "a key is emitted at most once per document")`).
If you have a case where the count comes out 2, I would like the document — the tier table in your comment suggests you saw something real, and if the guard has a hole I would rather find it than close this.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -375,14 +477,20 @@ private void writeDenseKeyColumn(String key)
RoaringBitmap presence = _presenceBitmaps.get(key);
List<Object> values = _values.get(key);
- // TODO: Honor the declared child field spec (field type, single/multi-value and custom default null value) instead
- // of synthesizing a single-value dimension of the stored type, so a document without the key reads the same as
- // through OpenStructDataSource.getValueFieldSpec, which returns the declared spec for a key absent from the
- // segment. See https://github.com/apache/pinot/issues/19466
- // Synthetic field spec for the materialized child. Its natural Pinot dimension null value is the value
- // stored for absent docs, so column metadata stays consistent with on-disk content.
- DimensionFieldSpec childFieldSpec = new DimensionFieldSpec(materializedCol, storedType, true);
- Object defaultValue = childFieldSpec.getDefaultNullValue();
+ boolean singleValue = !_multiValueKeys.contains(key);
Review Comment:
The sparse tier does carry the shape — via the `sparseMultiValueKeys` manifest, which is what your comment on `ImmutableOpenStructDataSource` L133 is looking at.
`_multiValueKeys` is read on the sparse path too: `OpenStructColumnSplitter` L823-834 writes `sparseMultiValueKeys` (key → max length) for every sparse key it holds, into `SPARSE_MULTI_VALUE_KEYS`. It is read back through `ColumnMetadataImpl` L475-480 and reaches `ImmutableOpenStructDataSource` L132: `boolean singleValue = _sparseMultiValueKeys == null || !_sparseMultiValueKeys.containsKey(key);` — so the undeclared sparse key gets an MV `DimensionFieldSpec`, and `SparseKeyDataSource` takes both shape (L81, L104) and length (L438) from it.
So `col[\"tags\"] = \"a\"` matches on both tiers and the shape does not flip with `maxDenseKeys`. Agreed that a sparse case in `OpenStructMultiValueKeyTest` would pin it rather than leaving it to the manifest plumbing — happy to add one.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -196,62 +212,125 @@ 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.
+ Object[] elements = OpenStructTypeInference.asMultiValue(rawValue);
Review Comment:
It runs on a key’s first sighting, not every row: the call sits in the `else` of `if (_presenceBitmaps.containsKey(key))` (L239-251), and every later row takes the `_multiValueKeys.contains(key)` set lookup at L240. Declared keys never reach it at all, since it is behind `keySpec != null ? ... :` (L245-247).
There is a smaller version of your point though, and it is real: on that first sighting `asMultiValue(rawValue)` is evaluated twice, once at L247 and again at L252. I have left it for now because the second call is what the empty-list check needs and hoisting it tangles with the shape fix in this commit, but say the word and I will fold it in.
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "raghavyadav01 (via GitHub)" <gi...@apache.org>.
raghavyadav01 commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4118369177
##########
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:
You are right, and this one was mine — fixed in c10fc97.
The guard could not fire, for exactly the reason you give: the container arrives as JSON **text**, so `document.get(prefix)` is a `String` and the `instanceof Map` check never matched. It only worked on the path where the blob is parsed back into real maps, which is not the native one.
And your read of the tests is the part that stung: they stubbed `openMapValueReader()` with hand-built `LinkedHashMap`s, a shape reconstruction never produces. I had a `jsonOrText` helper doing this unwrap in an earlier revision and dropped it when I moved the assembly onto `openMapValueReader()`; the stubs were what let that go unnoticed.
Two changes:
- `renderValue` unwraps a `String` that parses as an object or an array, recursively, so a container nested two deep comes back nested rather than escaped.
- the dedup check no longer asks only whether a prefix *is* a container — it asks whether that container actually holds the rest of the path. Without that, a string that merely looks like JSON could swallow a key genuinely spelled with a dot.
New tests use the real shape (container as text alongside its dotted leaves), plus cases for recursive unwrapping, a JSON-looking string that must not swallow `a.b`, non-JSON text staying text, and a leaf its container does not hold being kept rather than dropped. Correcting the old fixture also surfaced that the container in it was missing one of its own leaves, which the stricter check now catches.
##########
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:
Not deliberate — shape got persisted and type did not, and you are right that fix 5 is what made the disagreement visible.
To your question of whether it is worth persisting alongside the shape in the same manifest: I think yes, and it is the same manifest and the same write site (`OpenStructColumnSplitter` L823-834 already walks the sparse keys and has the inferred type in hand), so it is a small change rather than a new mechanism.
I would rather not fold it into this PR — it changes what `SPARSE_MULTI_VALUE_KEYS` carries, which is a format change and deserves its own review and its own consuming/sealed parity test, and nothing here regresses without it. Happy to do it as the immediate follow-up unless you would rather see it land together.
--
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
Re: [PR] OPEN_STRUCT fixes: nested paths, multi-value keys, declared types, sparse blob reads, and whole-column projection [pinot]
Posted by "tarun11Mavani (via GitHub)" <gi...@apache.org>.
tarun11Mavani commented on code in PR #19608:
URL: https://github.com/apache/pinot/pull/19608#discussion_r4119358443
##########
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:
I think follow up should be fine as it's not directly introduced in this PR.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/openstruct/OpenStructColumnSplitter.java:
##########
@@ -196,62 +212,125 @@ 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.
+ Object[] elements = OpenStructTypeInference.asMultiValue(rawValue);
Review Comment:
Since it's first sighting rather than each row, I think it should be fine.
--
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
Re: [PR] Materialize an OPEN_STRUCT key holding lists as a multi-value column [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19608:
URL: https://github.com/apache/pinot/pull/19608#issuecomment-5752932283
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19608?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `147 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.91%. Comparing base ([`be6eb51`](https://app.codecov.io/gh/apache/pinot/commit/be6eb519c658bfbc08a71113768417dd2e17a5bb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`847df52`](https://app.codecov.io/gh/apache/pinot/commit/847df524907e5f7ebf0c984508410e202e117c90?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 12 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19608?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ator/impl/openstruct/OpenStructColumnSplitter.java](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2Fopenstruct%2FOpenStructColumnSplitter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9vcGVuc3RydWN0L09wZW5TdHJ1Y3RDb2x1bW5TcGxpdHRlci5qYXZh) | 0.00% | [51 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...gment/index/openstruct/MutableOpenStructIndex.java](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Fopenstruct%2FMutableOpenStructIndex.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L29wZW5zdHJ1Y3QvTXV0YWJsZU9wZW5TdHJ1Y3RJbmRleC5qYXZh) | 0.00% | [50 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...cal/segment/index/openstruct/MutableKeyColumn.java](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Fopenstruct%2FMutableKeyColumn.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L29wZW5zdHJ1Y3QvTXV0YWJsZUtleUNvbHVtbi5qYXZh) | 0.00% | [29 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/pinot/spi/data/OpenStructTypeInference.java](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FOpenStructTypeInference.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9PcGVuU3RydWN0VHlwZUluZmVyZW5jZS5qYXZh) | 0.00% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19608?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (be6eb51) and HEAD (847df52). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (be6eb51) | HEAD (847df52) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|unittests2|1|0|
>|lane-a|2|0|
>|integration|4|0|
>|integration2|2|0|
>|lane-b|2|0|
>|integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19608 +/- ##
============================================
- Coverage 67.82% 57.91% -9.91%
+ Complexity 1450 1 -1449
============================================
Files 3503 2695 -808
Lines 226550 165825 -60725
Branches 35799 26999 -8800
============================================
- Hits 153662 96041 -57621
- Misses 60774 61665 +891
+ Partials 12114 8119 -3995
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.91% <0.00%> (-9.91%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.91% <0.00%> (-9.91%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.91% <0.00%> (-9.91%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.91% <0.00%> (-0.08%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19608/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19608?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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