clintropolis commented on code in PR #18539:
URL: https://github.com/apache/druid/pull/18539#discussion_r2361213594
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -235,7 +235,7 @@ public boolean isCompatibleOrdering(List<OrderBy> ordering)
return false;
}
for (int i = 0; i < preferredOrdering.size(); i++) {
- if (!ordering.get(i).equals(preferredOrdering.get(i))) {
+ if
(!ordering.get(i).getColumnName().equals(preferredOrdering.get(i).getColumnName()))
{
Review Comment:
I don't think we want to change this method since i think some callers do
need to check that the directions are the same.
More importantly though, this needs to be more strict than just matching
name, the check that projections needs is that the ordering is either same
direction, or every column is exactly the opposite direction of the preferred
ordering. So like `ORDER BY X ASC, Y DESC` can match `X ASC, Y DESC` or `X
DESC, Y ASC`, but cannot match `X ASC Y ASC`.
This functionality is not really used much yet since today mainly the only
thing callers care about is ordered by __time first, but we want stuff to be
correct in the future as we expand
##########
processing/src/test/java/org/apache/druid/segment/CursorFactoryProjectionTest.java:
##########
@@ -605,6 +590,10 @@ public void testProjectionSelectionTwoDims()
.setInterval(Intervals.ETERNITY)
.addDimension("a")
.addDimension("b")
+ .addOrderByColumn("a", Direction.DESCENDING)
+ .addOrderByColumn("b", Direction.DESCENDING)
+ .setLimit(10)
+ .setContext(Map.of(QueryContexts.QUERY_RESOURCE_ID,
RESOURCE_ID))
Review Comment:
if this is required for something, it should maybe be set by `testGroupBy`
or something instead of needing each test to set it specifically
--
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]
