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