xiangfu0 commented on code in PR #19879:
URL: https://github.com/apache/pinot/pull/19879#discussion_r4285734070
##########
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:
This defaults a new, behavior-changing query optimization to ON for
everyone. The project convention (and the neighbouring `allowReverseOrder`,
`useMaterializedViewRewrite`, `usePhysicalOptimizer`) is that new features
default to OFF until they've baked for a release. Enabling by default also
silently changes the observable `numEntriesScannedPostFilter` metric on these
queries, which dashboards/alerts may track. Could we default this to `false`
and flip it later once it's proven in the wild?
##########
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:
Minor: `numColumns` counts distinct physical columns, but if a non-order-by
expression references a column that's also the order-by column, that column
gets re-read here and counted again. Harmless for correctness, just a small
over-read in the entries-scanned accounting. Worth a one-line note, no change
required.
--
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]
