raghavyadav01 commented on code in PR #19040:
URL: https://github.com/apache/pinot/pull/19040#discussion_r3678929512
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -204,6 +222,55 @@ private boolean isFitForNonScanBasedPlan() {
return true;
}
+ @Nullable
+ private DataSource resolveDataSource(ExpressionContext expression) {
+ return resolveDataSource(expression, _indexSegment,
_queryContext.getSchema());
+ }
+
+ @Nullable
+ static DataSource resolveDataSource(ExpressionContext expression,
IndexSegment segment,
+ @Nullable org.apache.pinot.spi.data.Schema schema) {
+ if (expression.getType() == ExpressionContext.Type.IDENTIFIER) {
+ return segment.getDataSource(expression.getIdentifier(), schema);
+ }
+ if (expression.getType() == ExpressionContext.Type.FUNCTION) {
+ return tryResolveKeyedDataSource(expression, segment, schema);
+ }
+ return null;
+ }
+
+ @Nullable
+ static DataSource tryResolveKeyedDataSource(ExpressionContext expression,
IndexSegment segment,
+ @Nullable org.apache.pinot.spi.data.Schema schema) {
+ FunctionContext function = expression.getFunction();
+ if (function == null
+ ||
!ItemTransformFunction.FUNCTION_NAME.equals(function.getFunctionName())) {
+ return null;
+ }
+ List<ExpressionContext> args = function.getArguments();
+ if (args.size() != 2
+ || args.get(0).getType() != ExpressionContext.Type.IDENTIFIER
+ || args.get(1).getType() != ExpressionContext.Type.LITERAL) {
+ return null;
+ }
+ String columnName = args.get(0).getIdentifier();
+ String key = args.get(1).getLiteral().getStringValue();
+ DataSource columnDs = segment.getDataSource(columnName, schema);
+ if (columnDs instanceof MapDataSource) {
+ DataSource keyDs = ((MapDataSource) columnDs).getDataSource(key);
+ // An absent MAP key yields a NullDataSource whose metadata reports
non-null min/max (the INT
+ // default) and which carries no null vector, so it would wrongly
satisfy the metadata-based
+ // non-scan check and report hasNullValues=false. Fall back to a scan,
mirroring the
+ // OPEN_STRUCT guard below.
+ return keyDs instanceof NullDataSource ? null : keyDs;
+ }
+ if (columnDs instanceof OpenStructDataSource) {
+ OpenStructDataSource osDs = (OpenStructDataSource) columnDs;
+ return osDs.isMaterialized(key) ? osDs.getDataSource(key) : null;
Review Comment:
For a key present in only some docs, this routes MIN/MAX/DISTINCTCOUNT to
the non-scan path, which reads the dictionary — and the mutable dictionary
holds present values only. With null handling off, doesn't that diverge from
the scan path (and from sealed segments, which fold the default null in at
build time)? Might be worth a consuming-vs-sealed test to pin the expected
value.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/ProjectionBlock.java:
##########
@@ -59,15 +63,37 @@ public BlockValSet getBlockValueSet(ExpressionContext
expression) {
@Override
public BlockValSet getBlockValueSet(String column) {
- return new ProjectionBlockValSet(_dataBlockCache, column,
_dataSourceMap.get(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']");
+ }
+ return new ProjectionBlockValSet(_dataBlockCache, column, dataSource);
}
@Override
public BlockValSet getBlockValueSet(String[] paths) {
// TODO: only support one level of path for now, e.g. `map.key`
assert paths.length == 2;
- MapDataSource mapDataSource = (MapDataSource) _dataSourceMap.get(paths[0]);
- DataSource keyDataSource = mapDataSource.getDataSource(paths[1]);
+ DataSource columnDataSource = _dataSourceMap.get(paths[0]);
+ DataSource keyDataSource;
+ if (columnDataSource instanceof MapDataSource) {
+ keyDataSource = ((MapDataSource)
columnDataSource).getDataSource(paths[1]);
+ if (keyDataSource == null) {
+ keyDataSource = new NullDataSource(paths[1]);
+ }
+ } else if (columnDataSource instanceof OpenStructDataSource) {
+ OpenStructDataSource osDs = (OpenStructDataSource) columnDataSource;
+ keyDataSource = osDs.getDataSource(paths[1]);
+ if (keyDataSource == null) {
+ keyDataSource = OpenStructNullDataSource.forAbsentKey(osDs, paths[1]);
Review Comment:
For a sparse (not-yet-materialized) key, `getDataSource` returns null even
though the value may still live in the sparse blob, so this projects all-NULL
for every row. Should sparse keys fall back to the sparse/JSON read path here,
or is materialized-only the intended scope for this PR?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableOpenStructDataSource.java:
##########
@@ -62,13 +69,19 @@ public ComplexFieldSpec getFieldSpec() {
@Override
@Nullable
public DataSource getDataSource(String key) {
- Map<IndexType, IndexReader> indexes = _index.getIndexes(key);
- if (indexes == null || indexes.isEmpty()) {
+ // Live lookup, outside the memo: a key not yet observed may still be
created by the ingestion thread.
+ MutableKeyColumn col = _index.getKeyColumn(key);
+ if (col == null) {
return null;
}
- ColumnMetadata metadata = _index.getColumnMetadata(key);
- return new ImmutableDataSource(metadata,
- new ColumnIndexContainer.FromMap.Builder().withAll(indexes).build());
+ return _perKeyDataSourceCache.computeIfAbsent(key, k -> {
+ Map<IndexType, IndexReader> indexes = new
HashMap<>(_index.getIndexes(k));
Review Comment:
This hands the engine `MutableKeyColumn`'s raw forward index, which only has
slots written at present docIds (1000-row chunks). A full `[0, numDocs)`
projection/scan reads via `_readers.get(docId / 1000)` with no presence check —
so wouldn't an absent doc past the last present chunk hit
`_readers.get(bufferId)` out-of-bounds, and an in-range absent doc return
dictId `0` (the first inserted value)? Should the per-key read be
presence-aware (default for absent docs) here, and do we have a
consuming-segment test that projects/scans a key absent from the tail rows?
--
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]
