Add Sort Operator for Window Functions #13619
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sql/src/test/java/org/apache/druid/sql/calcite/CalciteWindowQueryTest.java
Fixed
Show fixed
Hide fixed
Add the NaiveSortMaker which makes a sorter object and a default implementation of the interface.
| @@ -891,6 +891,7 @@ public void assertResultsEquals(String sql, List<Object[]> expectedResults, List | |||
| results.get(i) | |||
| ); | |||
| } | |||
| Assert.assertEquals(expectedResults.size(), results.size()); | |||
There was a problem hiding this comment.
This change seems fine. Didn't look at the rest of the PR.
|
|
||
| // Rename the file and the file's extension from .tmp to .gz to prevent issues with 'parsing' the file | ||
| toRead = new File(tempFile.getParentFile(), "unparseable.gz"); | ||
| tempFile.renameTo(toRead); |
Check notice
Code scanning / CodeQL
Ignored error status of call
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class ColumnWithDirection |
There was a problem hiding this comment.
sort of sad to have another one of these (ScanQuery.OrderBy, OrderByColumnSpec of the group by query). It would be nice to standardize these someday...
There was a problem hiding this comment.
Yeah, I started using OrderByColumnSpec, it had a comparator on it that I was ignoring and it didn't have a good way to get at the comparator with the direction for me, so I gave up on it.
I wasn't aware of ScanQuery.OrderBy, that one looks closer to what I need and could maybe be adjusted to have the comparator multiplier on it. I'm not quite sure what NONE means for an ordering though...
I agree that we should not have so many, can probably look to converge what I have here with ScanQuery.OrderBy.
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class RearrangedRowsAndColumns implements RowsAndColumns |
There was a problem hiding this comment.
why "rearranged" instead of "sorted"? I don't really care, just curious 😅
also javadocs
There was a problem hiding this comment.
Because there's no guarantee that the re-arrangement is actually a sort.
| * Makes the NaiveSorter that will actually do the sort. This method uses {@code List<OrderByColumnSpec>} to avoid | ||
| * littering the code with extra objects for the same thing. {@code OrderByColumnSpec} is only used to identify |
There was a problem hiding this comment.
ah, I see that maybe you were originally using one of the other "order by" things, specifically the group by version? I guess the 'none' probably makes the scan query order by inappropriate too?
anyway, javadocs are stale
There was a problem hiding this comment.
Thanks for the callout.
| import java.util.LinkedHashMap; | ||
| import java.util.Map; | ||
|
|
||
| public class ConcatRowsAndColumns implements RowsAndColumns |
Adds the
NaiveSortMakerandNaiveSortOperatorwhich handle sorting.This adds a default implementation of the sort operation as well.
Additionally,
SortedGroupPartitionerhas been renamed toClusteredGroupPartitioneras the semantics actually don't require sorting (which requires definition of a direction).This PR has: