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