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