raghavyadav01 commented on code in PR #19040:
URL: https://github.com/apache/pinot/pull/19040#discussion_r3659751232
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/ItemTransformFunction.java:
##########
@@ -87,6 +103,11 @@ public Dictionary getDictionary() {
return _dictionary;
}
+ @Override
+ public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
Review Comment:
This override now applies to MAP `item()` too, not just OPEN_STRUCT. On
master `ItemTransformFunction` inherits `BaseTransformFunction.getNullBitmap`,
which ORs the argument bitmaps (effectively never-null for a map key).
Returning the per-key val-set bitmap silently changes `IS NULL` / null-aware
results for existing `map['key']` queries. Was that MAP behavior change
intended, and is it covered by a MAP-specific test?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/MutableOpenStructDataSource.java:
##########
@@ -62,10 +63,13 @@ public ComplexFieldSpec getFieldSpec() {
@Override
@Nullable
public DataSource getDataSource(String key) {
- Map<IndexType, IndexReader> indexes = _index.getIndexes(key);
- if (indexes == null || indexes.isEmpty()) {
+ MutableKeyColumn col = _index.getKeyColumn(key);
+ if (col == null) {
return null;
}
+ Map<IndexType, IndexReader> indexes = new
HashMap<>(_index.getIndexes(key));
Review Comment:
This allocates a fresh `HashMap` copy plus a new
`PresenceBasedNullValueVector` on every `getDataSource(key)` call, and
`getNullBitmap()` then rebuilds a full `[0,numDocs)` bitmap + clone per
invocation. If this is hit per-predicate on the query path, is it worth caching
the per-key DataSource, or is getDataSource already called once per query?
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -204,6 +221,50 @@ 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) {
+ return ((MapDataSource) columnDs).getDataSource(key);
Review Comment:
This now makes `item(mapCol, 'k')` eligible for the non-scan aggregation
path (previously any FUNCTION arg forced a scan). Two concerns for a key
present in only some docs: (1) `BaseMapDataSource.getDataSource` returns a
`NullDataSource` (INT, numDocs=0) for an absent key rather than null, so a
non-scan COUNT/MIN/MAX could run over 0 docs; (2) the per-key MAP source may
carry no null vector, so `hasNullValues` returns false and non-scan agg would
ignore absent-doc nulls. Can you confirm the per-key MAP source models
absent-doc nulls / full segment numDocs here?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/PresenceBasedNullValueVector.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * 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.segment.local.segment.index.openstruct;
+
+import org.apache.pinot.segment.spi.index.reader.NullValueVectorReader;
+import org.roaringbitmap.buffer.ImmutableRoaringBitmap;
+import org.roaringbitmap.buffer.MutableRoaringBitmap;
+
+
+/// {@link NullValueVectorReader} backed by an OPEN_STRUCT key's presence
bitmap.
+///
+/// A document is null for a key when the key was never set on that document,
i.e. when the
+/// document is absent from the presence bitmap. The null bitmap is computed
on demand (not cached)
+/// because the presence bitmap is mutable during real-time consumption.
+public class PresenceBasedNullValueVector implements NullValueVectorReader {
+ private final ImmutableRoaringBitmap _presenceBitmap;
+ private final int _numDocs;
+
+ public PresenceBasedNullValueVector(ImmutableRoaringBitmap presenceBitmap,
int numDocs) {
+ _presenceBitmap = presenceBitmap;
+ _numDocs = numDocs;
+ }
+
+ @Override
+ public boolean isNull(int docId) {
+ return !_presenceBitmap.contains(docId);
Review Comment:
`getNullBitmap()` below clones the presence bitmap before iterating to avoid
racing the ingestion thread, but `isNull()` reads the live mutable bitmap
directly via `contains()`. On a consuming segment, isn't this the same
concurrent-mutation hazard the clone was added to guard against? Should isNull
go through the same protection (or is contains() safe against a concurrent
`add`)?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/openstruct/ImmutableOpenStructDataSource.java:
##########
@@ -84,8 +84,7 @@ public ComplexFieldSpec getFieldSpec() {
@Override
@Nullable
public DataSource getDataSource(String key) {
- DataSource ds = _perKeyDataSources.get(key);
- return ds != null ? ds : _sparseDataSource;
+ return _perKeyDataSources.get(key);
Review Comment:
This flips `getDataSource(key)` from returning the `$__sparse__` fallback to
returning `null` for unmaterialized keys — a semantic change to a method other
call sites may have assumed non-null. Have all existing callers been audited to
handle null (vs. the old sparse fallback)?
--
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]
