arunkumarucet commented on code in PR #19879:
URL: https://github.com/apache/pinot/pull/19879#discussion_r4291005452
##########
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";
Review Comment:
I agree with the principle, and I'd like to get there — just not in this PR.
Two things pull against each other here.
Your argument is sound: the unsorted ORDER BY path already late-materializes
with no query option, so an option on the sorted path is an inconsistency, and
every flag is an execution mode that has to be kept tested for as long as it
exists.
But @xiangfu0 raised a concrete regression on the planner gate: when `limit
+ offset` reaches the size of the match set, late materialization reads the
same number of entries as the eager path and adds a whole second pass on top —
a bitmap build, a sort, an array copy, and a second decode/transform over every
surviving row. That shape is real (pagination and export queries), and the
internal eligibility check doesn't catch it, because that check is about
whether the scan is bounded by `LIMIT`, not about how large the limit is. With
no flag there'd be no escape hatch for someone who hits it.
So I've shipped it default-off in `c4667fa792`, which also answers Xiang's
request. What I'd propose:
1. This PR lands opt-in, with the eligibility check you asked to keep.
2. I benchmark where the crossover actually is — at what ratio of `limit +
offset` to match-set size the second pass stops paying for itself.
3. If the regression turns out to be negligible, the flag comes out in a
follow-up and this becomes automatic for eligible plans, exactly as you
describe. If it isn't negligible, the numbers tell us what the eligibility
check should test, and the flag can still come out once the check covers it.
That ordering is reversible: removing the flag later is easy, whereas
shipping with no flag and discovering the regression in the wild needs another
release to undo. Happy to file the follow-up issue now so it doesn't get lost.
If you'd rather have it automatic in this PR regardless, say so and I'll make
the change — I don't want to over-rotate on an edge case if you think it isn't
one.
--
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]
