Akshat-Jain opened a new pull request, #16729:
URL: https://github.com/apache/druid/pull/16729
### Description
This PR fixes some issues with MSQ window functions.
## Issue 1: NPE issues when multiple windows are used
Currently, queries like the following run into a NPE when using MSQ:
```sql
select countryName, cityName, channel,
row_number() over (partition by cityName order by countryName, cityName,
channel) as c1,
count(channel) over (partition by cityName order by countryName, cityName,
channel) as c2
from wikipedia
where countryName in ('Austria', 'Republic of Korea')
group by countryName, cityName, channel
```
because the `List<OperatorFactory>` we get in `WindowOperatorQueryKit` layer
is `[Sort, Partition, Window1, Window2]`.
WindowOperatorQueryKit was trying to group the window factories into
different groups, and was asserting a partition operator factory to be present
in each group, which wasn't valid, and ended up giving NPE.
## Issue 2: Query correctness issues because of incorrect trimming of row
signature
Currently, queries like the following give incorrect results when using MSQ:
```sql
select cityName, countryName,
row_number() over (partition by countryName order by countryName, cityName,
channel) as c1,
count(channel) over (partition by cityName order by countryName, cityName,
channel) as c2
from wikipedia
where countryName in ('Austria', 'Republic of Korea')
group by countryName, cityName, channel
```
because we are incorrectly trimming the row signature in
`WindowOperatorQueryKit` in the following part of code:
```java
final int numberOfWindows = operatorList.size();
final int baseSize = rowSignature.size() - numberOfWindows;
for (int i = 0; i < baseSize; i++) {
bob.add(rowSignature.getColumnName(i),
rowSignature.getColumnType(i).get());
}
for (int i = 0; i < numberOfWindows; i++) {
bob.add(rowSignature.getColumnName(baseSize + i),
rowSignature.getColumnType(baseSize + i).get()).build();
// rest of the code
}
```
## Solution
Issue 1 is fixed by removing the assertion, and returning null when no
partition operator factory is present for the current window stage evaluation.
This indicates that we already have the data partitioned correctly, and hence
we don't need to do any shuffling.
Issue 2 is fixed by revamping the logic of computing the row signature for
every window stage. Changes done to achieve this:
1. Added a `getOutputColumnNames()` method in `Processor` interface
2. Calculating the list of partition column names for a given window stage,
and passing that list to the window processor layer, for usage in
`WindowOperatorQueryFrameProcessor#comparePartitionKeys`.
## Test Plan
1. Added DruidWindowQueryTest and MSQDruidWindowQueryTest, which use the
same wiring done in DrillWindowQueryTest and MSQDrillWindowQueryTest. The
common functionality was moved into a base class `WindowQueryTestBase`. We
decided to create a new layer as we didn't want to add non-drill tests into the
existing drill test layer.
2. Added a few category of tests in DruidWindowQueryTest (and hence also in
MSQDruidWindowQueryTest).
3. Did a lot of manual testing as well.
<hr>
##### Key changed/added classes in this PR
* `WindowOperatorQueryKit`
* `WindowOperatorQueryFrameProcessor `
* Test files: `DruidWindowQueryTest `, `MSQDruidWindowQueryTest`,
`DrillWindowQueryTest`, `MSQDrillWindowQueryTest`, `WindowQueryTestBase`
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked
related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in
[licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [x] added comments explaining the "why" and the intent of the code
wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths,
ensuring the threshold for [code
coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md)
is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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]
