xiangfu0 commented on code in PR #19040:
URL: https://github.com/apache/pinot/pull/19040#discussion_r3632306646
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ItemTransformFunction.java:
##########
@@ -58,9 +60,17 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
_keyPath = new String[]{column, key};
DataSource dataSource = columnContextMap.get(column).getDataSource();
- Preconditions.checkState(dataSource instanceof MapDataSource, "Column: %s
must be a MAP column", column);
- MapDataSource mapDataSource = (MapDataSource) dataSource;
- DataSource valueDataSource = mapDataSource.getDataSource(key);
+ Preconditions.checkState(dataSource instanceof MapDataSource || dataSource
instanceof OpenStructDataSource,
+ "Column: %s must be a MAP or OPEN_STRUCT column", column);
+ DataSource valueDataSource;
+ if (dataSource instanceof MapDataSource) {
+ valueDataSource = ((MapDataSource) dataSource).getDataSource(key);
+ } else {
+ valueDataSource = ((OpenStructDataSource) dataSource).getDataSource(key);
Review Comment:
Sparse OPEN_STRUCT keys do not have a per-key DataSource here.
`ImmutableOpenStructDataSource#getDataSource()` returns the shared
`$__sparse__` JSON column for every unmaterialized key, so the expression
fallback compares or projects the entire JSON blob instead of the requested
value. For example, `metrics['region'] = 'us'` compares `{"region":"us",...}`
with `us` and silently returns no rows. Only use the direct DataSource path for
`isMaterialized(key)` and add a key-aware sparse extractor or JSON delegate.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/MapFilterOperator.java:
##########
@@ -67,104 +79,161 @@ public MapFilterOperator(IndexSegment indexSegment,
Predicate predicate, QueryCo
_columnName = arguments.get(0).getIdentifier();
_keyName = arguments.get(1).getLiteral().getStringValue();
- JsonIndexReader jsonIndex = null;
- if (canUseJsonIndex(_predicate.getType())) {
- DataSource dataSource = indexSegment.getDataSourceNullable(_columnName);
- if (dataSource != null) {
- jsonIndex = dataSource.getJsonIndex();
- if (jsonIndex == null) {
- // Fallback to Composite JSON Index if standard JSON index is not
available
- Optional<IndexType<?, ?, ?>> compositeIndex =
- IndexService.getInstance().getOptional("composite_json_index");
- if (compositeIndex.isPresent()) {
- jsonIndex = (JsonIndexReader)
dataSource.getIndex(compositeIndex.get());
- }
- }
- }
+ // Try dispatch paths in priority order
+ DataSource columnDs = indexSegment.getDataSourceNullable(_columnName);
+
+ BaseFilterOperator perKey = tryPerKeyIndex(columnDs, queryContext,
numDocs);
+ if (perKey != null) {
+ _delegate = perKey;
+ _delegateType = DelegateType.PER_KEY_INDEX;
+ return;
}
- if (jsonIndex != null) {
- FilterContext filterContext = createFilterContext();
- _jsonMatchOperator = new JsonMatchFilterOperator(jsonIndex,
filterContext, numDocs);
- _expressionFilterOperator = null;
- } else {
- _jsonMatchOperator = null;
- _expressionFilterOperator = new ExpressionFilterOperator(indexSegment,
queryContext, predicate, numDocs);
+
+ JsonMatchFilterOperator jsonOp = tryJsonIndex(columnDs, numDocs);
+ if (jsonOp != null) {
+ _delegate = jsonOp;
+ _delegateType = DelegateType.JSON_MATCH;
+ return;
}
+
+ _delegate = new ExpressionFilterOperator(indexSegment, queryContext,
predicate, numDocs);
+ _delegateType = DelegateType.EXPRESSION_FILTER;
}
- /**
- * Creates a FilterContext based on the original predicate type
- */
- private FilterContext createFilterContext() {
- // Create identifier expression for the JSON column
- ExpressionContext keyLhs = ExpressionContext.forIdentifier(_keyName);
+ @Nullable
+ private BaseFilterOperator tryPerKeyIndex(@Nullable DataSource columnDs,
QueryContext queryContext, int numDocs) {
+ if (!(columnDs instanceof OpenStructDataSource)) {
+ return null;
+ }
+ OpenStructDataSource osDs = (OpenStructDataSource) columnDs;
- // Create predicate based on type
- Predicate predicate;
+ if (osDs.isMaterialized(_keyName)) {
+ DataSource keyDs = osDs.getDataSource(_keyName);
+ return buildPerKeyFilterOperator(keyDs, queryContext, numDocs);
+ }
+
+ // Key not materialized
+ if (osDs.isFullyMaterialized()) {
+ // Fully materialized but key absent — definitive answer
+ if (_predicate.getType() == Predicate.Type.IS_NULL) {
+ return new MatchAllFilterOperator(numDocs);
+ }
+ return EmptyFilterOperator.getInstance();
+ }
+
+ // Sparse — can't be sure, fall through to JSON/expression
+ return null;
+ }
+
+ @Nullable
+ private BaseFilterOperator buildPerKeyFilterOperator(DataSource keyDs,
QueryContext queryContext, int numDocs) {
switch (_predicate.getType()) {
+ case IS_NULL:
+ case IS_NOT_NULL: {
+ NullValueVectorReader nullReader = keyDs.getNullValueVector();
+ if (nullReader == null) {
Review Comment:
The assumption that a missing null vector means every value is non-null is
false for consuming OPEN_STRUCT segments. `MutableKeyColumn` tracks per-key
presence, but `MutableOpenStructIndex#getIndexes` exposes only forward,
dictionary, and inverted indexes, so this null reader is absent even when the
key exists in only some documents. `IS_NULL` consequently returns empty and
`IS_NOT_NULL` matches every document. Expose a presence-backed
`NullValueVectorReader` from mutable child DataSources before enabling this
fast path.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ItemTransformFunction.java:
##########
@@ -58,9 +60,17 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
_keyPath = new String[]{column, key};
DataSource dataSource = columnContextMap.get(column).getDataSource();
- Preconditions.checkState(dataSource instanceof MapDataSource, "Column: %s
must be a MAP column", column);
- MapDataSource mapDataSource = (MapDataSource) dataSource;
- DataSource valueDataSource = mapDataSource.getDataSource(key);
+ Preconditions.checkState(dataSource instanceof MapDataSource || dataSource
instanceof OpenStructDataSource,
Review Comment:
The new OPEN_STRUCT path inherits `BaseTransformFunction#getNullBitmap()`,
which only combines the null bitmaps of the parent `metrics` argument and key
literal. The parent OPEN_STRUCT DataSource has no null vector, so the selected
dense child's null bitmap is lost. Rows where the key is absent are emitted as
default values and treated as non-null, causing wrong projections, composed
expressions, and aggregates such as `COUNT(metrics['views'])` with null
handling enabled. Override `getNullBitmap()` to return the selected path's
BlockValSet null bitmap.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ItemTransformFunction.java:
##########
@@ -58,9 +60,17 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
_keyPath = new String[]{column, key};
DataSource dataSource = columnContextMap.get(column).getDataSource();
- Preconditions.checkState(dataSource instanceof MapDataSource, "Column: %s
must be a MAP column", column);
- MapDataSource mapDataSource = (MapDataSource) dataSource;
- DataSource valueDataSource = mapDataSource.getDataSource(key);
+ Preconditions.checkState(dataSource instanceof MapDataSource || dataSource
instanceof OpenStructDataSource,
+ "Column: %s must be a MAP or OPEN_STRUCT column", column);
+ DataSource valueDataSource;
+ if (dataSource instanceof MapDataSource) {
+ valueDataSource = ((MapDataSource) dataSource).getDataSource(key);
+ } else {
+ valueDataSource = ((OpenStructDataSource) dataSource).getDataSource(key);
+ }
+ if (valueDataSource == null) {
+ valueDataSource = new NullDataSource(key);
Review Comment:
`NullDataSource` is not a type-correct all-null representation for an absent
OPEN_STRUCT key. It is hard-coded as INT, has no null vector, and reports zero
documents, so a schema-declared STRING or DOUBLE key missing from one segment
becomes INT/0 there while other segments use its actual type. This can corrupt
`COUNT`/`MIN`/`MAX` and produce incompatible per-segment result schemas.
Construct a typed all-null DataSource from the OPEN_STRUCT child FieldSpec with
a null bitmap covering the segment.
--
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]
