-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSQ window functions: Reject MVDs during window processing #17036
Conversation
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java
Fixed
Show fixed
Hide fixed
...age-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java
Show resolved
Hide resolved
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java
Outdated
Show resolved
Hide resolved
Based on offline discussion, we can wait for GlueingPartitioningOperator to be used, which would make it return a proper error message. So I'll close this PR. |
We had some queries like the following giving incorrect results, because the MVDs (List object) were getting converted to string: As per offline discussion with @sreemanamala, have opened this PR and have added another check in This is a temporary measure and would be removed once we have the GlueingPartitioningOperator approach as mentioned previously. |
...age-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java
Show resolved
Hide resolved
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java
Fixed
Show resolved
Hide resolved
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQWindowTest.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how frequently people will run into this - but the updates which supposed to be made to this part will make this check as well go away.
Let's have it for now
@@ -128,7 +130,16 @@ public boolean isNull(int rowNum) | |||
@Override | |||
public Object getObject(int rowNum) | |||
{ | |||
return accessor.getObject(pointers[start + rowNum]); | |||
Object value = accessor.getObject(pointers[start + rowNum]); | |||
if (ColumnType.STRING.equals(getType()) && value instanceof List) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we require this check for druid 31, we can live with this "hackery". Once #17038 gets merged, please remove this check.
cc @Akshat-Jain
Description
This PR aims to reject MVDs when used as
partition by
columns during window function processing for MSQ engine.Prior to this change, a MSQ query like the following was failing with ClassCastException:
This is a follow-up on #17002, where a similar thing was done for sql-native engine.
This PR has: