kgyrtkirk commented on code in PR #16754:
URL: https://github.com/apache/druid/pull/16754#discussion_r1686682429
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryKit.java:
##########
@@ -136,7 +139,6 @@ public QueryDefinition makeQueryDefinition(
queryToRun,
queryToRun.getOperators(),
rowSignature,
- true,
maxRowsMaterialized,
new ArrayList<>()
Review Comment:
`Collections.emptyList`
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryFrameProcessor.java:
##########
@@ -484,7 +479,8 @@ private void convertRowFrameToRowsAndColumns(Frame frame)
/**
* Compare two rows based on the columns in partitionColumnNames.
- * If the partitionColumnNames is empty or null, compare entire row.
+ * If the partitionColumnNames is null, compare entire row.
Review Comment:
why add that extra meaning to null? its not really realistic to think that
all columns will be partition columns...
I don't think its used&tested - instead of allowing null make sure that the
`partitionColumnNames` field is never null after the constructor was called -
so that there is no chance to get an NPE in L187 either
--
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: commits-unsubscr...@druid.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
