xiangfu0 commented on code in PR #19866:
URL: https://github.com/apache/pinot/pull/19866#discussion_r4278277186
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/SelectionPlanNode.java:
##########
@@ -169,7 +169,10 @@ private int
getSortedColumnsPrefix(List<OrderByExpressionContext> orderByExpress
private boolean isSorted(OrderByExpressionContext orderByExpression, boolean
asc, boolean isNullHandlingEnabled) {
switch (orderByExpression.getExpression().getType()) {
case LITERAL: {
- return true;
+ // LITERAL expressions (e.g. unresolved positional ORDER BY 2, or
ORDER BY 1) are not backed by a data source
+ // column and therefore cannot rely on the segment's natural sort
order. Returning false ensures these queries
+ // fall through to the generic SelectionOrderByOperator rather than
taking the partial-order optimized path.
Review Comment:
The fix itself is correct, but this behavioral bug fix has no accompanying
test. Could you add a regression test in
`InnerSegmentSelectionSingleValueQueriesTest` that asserts an `ORDER BY
<literal>` query now yields a `SelectionOrderByOperator` (not one of the
`SelectionPartiallyOrderedBy*` operators) and no longer throws? That file
already has the operator-type assertion pattern, so it's a natural home and
would fail on the pre-fix code.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/SelectionPlanNode.java:
##########
@@ -169,7 +169,10 @@ private int
getSortedColumnsPrefix(List<OrderByExpressionContext> orderByExpress
private boolean isSorted(OrderByExpressionContext orderByExpression, boolean
asc, boolean isNullHandlingEnabled) {
switch (orderByExpression.getExpression().getType()) {
case LITERAL: {
Review Comment:
Heads up: the Javadoc on `getSortedColumnsPrefix` just above ("if they are
included, they are considered sorted and its ASC/DESC is ignored") now
describes the old behavior and contradicts this change. Please update it so a
future reader isn't misled — literals are now treated as unsorted and terminate
the prefix.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/SelectionPlanNode.java:
##########
@@ -169,7 +169,10 @@ private int
getSortedColumnsPrefix(List<OrderByExpressionContext> orderByExpress
private boolean isSorted(OrderByExpressionContext orderByExpression, boolean
asc, boolean isNullHandlingEnabled) {
switch (orderByExpression.getExpression().getType()) {
case LITERAL: {
Review Comment:
Heads up: the Javadoc on `getSortedColumnsPrefix` just above ("if they are
included, they are considered sorted and its ASC/DESC is ignored") now
describes the old behavior and contradicts this change. Please update it so a
future reader isn't misled — literals are now treated as unsorted and terminate
the prefix.
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/SelectionPlanNode.java:
##########
@@ -169,7 +169,10 @@ private int
getSortedColumnsPrefix(List<OrderByExpressionContext> orderByExpress
private boolean isSorted(OrderByExpressionContext orderByExpression, boolean
asc, boolean isNullHandlingEnabled) {
switch (orderByExpression.getExpression().getType()) {
case LITERAL: {
- return true;
+ // LITERAL expressions (e.g. unresolved positional ORDER BY 2, or
ORDER BY 1) are not backed by a data source
+ // column and therefore cannot rely on the segment's natural sort
order. Returning false ensures these queries
+ // fall through to the generic SelectionOrderByOperator rather than
taking the partial-order optimized path.
Review Comment:
The fix itself is correct, but this behavioral bug fix has no accompanying
test. Could you add a regression test in
`InnerSegmentSelectionSingleValueQueriesTest` that asserts an `ORDER BY
<literal>` query now yields a `SelectionOrderByOperator` (not one of the
`SelectionPartiallyOrderedBy*` operators) and no longer throws? That file
already has the operator-type assertion pattern, so it's a natural home and
would fail on the pre-fix code.
--
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]
