arunkumarucet commented on code in PR #19879:
URL: https://github.com/apache/pinot/pull/19879#discussion_r4290973516
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1048,6 +1048,11 @@ public static class QueryOptionKey {
public static final String ALLOW_REVERSE_ORDER = "allowReverseOrder";
public static final boolean DEFAULT_ALLOW_REVERSE_ORDER = false;
+ /// Selection with an order-by prefix on a sorted column: project only
the order-by expressions during the
+ /// scan and fetch the remaining output expressions for the rows that
survive the limit. Disabling it
+ /// restores reading every selected column for the whole match set.
+ public static final String LATE_MATERIALIZE_SORTED_ORDER_BY =
"lateMaterializeSortedOrderBy";
+ public static final boolean DEFAULT_LATE_MATERIALIZE_SORTED_ORDER_BY =
true;
Review Comment:
Agreed, done in `c4667fa792` — the option now defaults to `false`. You're
right on both counts: it follows the convention of the neighbouring options,
and the silent `numEntriesScannedPostFilter` change is a real reason not to
flip it under people without warning. Added a test that pins the eager default
so the flip has to be deliberate, and documented in `CommonConstants` why it's
off.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/SelectionPlanNode.java:
##########
@@ -88,17 +88,38 @@ public Operator<SelectionResultsBlock> run() {
maxDocsPerCall = Math.min(limit + _queryContext.getOffset(),
DocIdSetPlanNode.MAX_DOC_PER_CALL);
}
- BaseProjectOperator<?> projectOperator = getSortedByProject(expressions,
maxDocsPerCall, orderByExpressions);
+ // Whether the scan can iterate in query order, and so stop once the
list builder is full. Ascending always
+ // can; descending only when reverse iteration is permitted, because
DocIdSetPlanNode only produces an
+ // ascending DocIdSetOperator. getSortedByProject below performs the
actual switch and decides for real; this
+ // is a conservative prediction, and being wrong only costs the
optimization, never correctness.
boolean asc = orderByExpressions.get(0).isAsc();
+ boolean canIterateInQueryOrder =
+ asc ||
QueryOptionsUtils.isReverseOrderAllowed(_queryContext.getQueryOptions());
+
+ // Projecting only the order-by expressions and fetching the rest for
the rows that survive the limit pays off
+ // only when the scan cannot stop at the limit. The linear operator
stops as soon as the list builder is full,
+ // which with a fully sorted order-by prefix is after limit+offset
documents, so there is nothing to save
+ // there. The descending operator has to read the whole match set
because there is no descending cursor.
+ boolean scanBoundedByLimit = canIterateInQueryOrder &&
sortedColumnsPrefixSize == orderByExpressions.size();
+ boolean lateMaterialize = expressions.size() > numOrderByExpressions &&
!scanBoundedByLimit
Review Comment:
Addressed by shipping default-off in `c4667fa792`, which is the second of
the two options you offered. Your analysis of the edge is right: once `limit +
offset` reaches the match-set size the second pass reads what the scan would
have read anyway, and adds a bitmap build, a sort, an array copy and a second
decode on top. I've noted that in the option's documentation. I'd rather not
add a selectivity guard yet — picking the threshold needs the benchmark numbers
we don't have, and opt-in means nobody meets the edge unasked. Happy to add one
when there's data to set it from.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/LinearSelectionOrderByOperator.java:
##########
@@ -124,39 +159,76 @@ public IndexSegment getIndexSegment() {
public ExecutionStatistics getExecutionStatistics() {
long numEntriesScannedInFilter =
_projectOperator.getExecutionStatistics().getNumEntriesScannedInFilter();
int numDocsScanned = getNumDocsScanned();
- long numEntriesScannedPostFilter = (long) numDocsScanned *
_projectOperator.getNumColumnsProjected();
+ long numEntriesScannedPostFilter =
+ (long) numDocsScanned * _projectOperator.getNumColumnsProjected() +
_numEntriesScannedPostFilterLate;
int numTotalDocs = _indexSegment.getSegmentMetadata().getTotalDocs();
return new ExecutionStatistics(numDocsScanned, numEntriesScannedInFilter,
numEntriesScannedPostFilter,
numTotalDocs);
}
+ /// Number of expressions the project operator actually produces: all of
them, or just the order-by prefix when
+ /// materialising late.
+ protected int getNumProjectedExpressions() {
+ return _columnContexts.length;
+ }
+
+ protected BlockValSet[] newBlockValSets() {
+ return new BlockValSet[getNumProjectedExpressions()];
+ }
+
protected IntFunction<Object[]> fetchBlock(ValueBlock valueBlock,
BlockValSet[] blockValSets) {
- int numExpressions = _expressions.size();
+ int numProjected = getNumProjectedExpressions();
- for (int i = 0; i < numExpressions; i++) {
+ for (int i = 0; i < numProjected; i++) {
ExpressionContext expression = _expressions.get(i);
blockValSets[i] = valueBlock.getBlockValueSet(expression);
}
RowBasedBlockValueFetcher blockValueFetcher = new
RowBasedBlockValueFetcher(blockValSets);
- if (!_nullHandlingEnabled) {
- return blockValueFetcher::getRow;
+ RoaringBitmap[] nullBitmaps;
+ if (_nullHandlingEnabled) {
+ nullBitmaps = new RoaringBitmap[numProjected];
+ for (int i = 0; i < numProjected; i++) {
+ nullBitmaps[i] = blockValSets[i].getNullBitmap();
+ }
+ } else {
+ nullBitmaps = null;
}
- RoaringBitmap[] nullBitmaps = new RoaringBitmap[numExpressions];
- for (int i = 0; i < numExpressions; i++) {
- nullBitmaps[i] = blockValSets[i].getNullBitmap();
+
+ if (!_lateMaterialize) {
+ if (nullBitmaps == null) {
+ return blockValueFetcher::getRow;
+ }
+ return (docId) -> {
+ Object[] row = blockValueFetcher.getRow(docId);
+ applyNulls(row, nullBitmaps, docId);
+ return row;
+ };
}
- return (docId) -> {
- Object[] row = blockValueFetcher.getRow(docId);
- for (int colId = 0; colId < nullBitmaps.length; colId++) {
- if (nullBitmaps[colId] != null && nullBitmaps[colId].contains(docId)) {
- row[colId] = null;
- }
+
+ // Leave room for the expressions fetched after the limit is applied, and
carry the doc id in the first of those
+ // slots so the fetch knows which documents to read. That slot is
overwritten by the first fetched value.
+ int numExpressions = _expressions.size();
+ int[] docIds = valueBlock.getDocIds();
+ return (rowId) -> {
+ Object[] row = new Object[numExpressions];
+ blockValueFetcher.getRow(rowId, row, 0);
+ if (nullBitmaps != null) {
+ applyNulls(row, nullBitmaps, rowId);
}
+ row[_numOrderByExpressions] = docIds[rowId];
return row;
};
}
+ private static void applyNulls(Object[] row, RoaringBitmap[] nullBitmaps,
int rowId) {
Review Comment:
Done in `c4667fa792` — renamed to `rowIdInBlock` in both lambdas and in
`applyNulls`. You're right that this is exactly the place to be unambiguous
about it.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/LinearSelectionOrderByOperator.java:
##########
@@ -234,7 +312,105 @@ private void concatList(StringBuilder sb, List<?> list) {
@Override
protected SelectionResultsBlock getNextBlock() {
- return new SelectionResultsBlock(createDataSchema(),
fetch(_listBuilderSupplier), _comparator, _queryContext);
+ List<Object[]> rows = fetch(_listBuilderSupplier);
+ if (!_lateMaterialize) {
+ return new SelectionResultsBlock(createDataSchema(), rows, _comparator,
_queryContext);
+ }
+ return fetchRemainingExpressions(rows);
+ }
+
+ /// Fills the output expressions that were not projected during the scan,
for the rows that survived the limit.
+ ///
+ /// The rows are visited in doc-id order because that is the order
[BitmapDocIdSetOperator#ascending] returns, while
+ /// `rows` keeps the result order. Both views hold the same row arrays, so
filling one fills the other.
+ private SelectionResultsBlock fetchRemainingExpressions(List<Object[]> rows)
{
+ int numExpressions = _expressions.size();
+ List<ExpressionContext> nonOrderByExpressions =
_expressions.subList(_numOrderByExpressions, numExpressions);
+ int numNonOrderByExpressions = nonOrderByExpressions.size();
+
+ int numRows = rows.size();
+ Object[][] rowsByDocId = rows.toArray(new Object[numRows][]);
+ Arrays.sort(rowsByDocId, Comparator.comparingInt(row -> (int)
row[_numOrderByExpressions]));
+ RoaringBitmap docIds = new RoaringBitmap();
+ for (Object[] row : rowsByDocId) {
+ docIds.add((int) row[_numOrderByExpressions]);
+ }
+
+ Set<String> columns = new HashSet<>();
+ for (ExpressionContext expression : nonOrderByExpressions) {
+ expression.getColumns(columns);
+ }
+ Map<String, DataSource> dataSourceMap = new HashMap<>();
+ for (String column : columns) {
+ dataSourceMap.put(column, _indexSegment.getDataSource(column,
_queryContext.getSchema()));
+ }
+ int numColumns = columns.size();
Review Comment:
Noted in `c4667fa792`. Added a comment where `numColumns` is computed saying
that a column which is both an order-by column and referenced by a non-order-by
expression is read again and counted again, so the accounting slightly
over-reports. No behaviour change, as you said.
--
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]
