Akshat-Jain commented on code in PR #16729:
URL: https://github.com/apache/druid/pull/16729#discussion_r1675841676


##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryKit.java:
##########
@@ -102,18 +116,18 @@ public QueryDefinition makeQueryDefinition(
     final int firstStageNumber = Math.max(minStageNumber, 
queryDefBuilder.getNextStageNumber());
     final WindowOperatorQuery queryToRun = (WindowOperatorQuery) 
originalQuery.withDataSource(dataSourcePlan.getNewDataSource());
     final int maxRowsMaterialized;
-    RowSignature rowSignature = queryToRun.getRowSignature();
+
     if (originalQuery.context() != null && 
originalQuery.context().containsKey(MultiStageQueryContext.MAX_ROWS_MATERIALIZED_IN_WINDOW))
 {
-      maxRowsMaterialized = (int) originalQuery.context()
-                                                         
.get(MultiStageQueryContext.MAX_ROWS_MATERIALIZED_IN_WINDOW);
+      maxRowsMaterialized = (int) 
originalQuery.context().get(MultiStageQueryContext.MAX_ROWS_MATERIALIZED_IN_WINDOW);
     } else {
       maxRowsMaterialized = Limits.MAX_ROWS_MATERIALIZED_IN_WINDOW;
     }
 
-
-    if (isEmptyOverFound) {
+    if (isEmptyOverPresent) {
       // empty over clause found
       // moving everything to a single partition
+      // TODO: This logic needs to be revamped and corrected in the future.
+      //  This should likely cause issues for cases where we have a mix of 
empty over() and non-empty over().

Review Comment:
   @sreemanamala I haven't dug into the scenario. I don't expect it to fail in 
all cases. I don't have an exhaustive list of cases where it might fail, so 
throwing an error in all cases would be overkill. Also, this isn't a regression 
caused because of this PR changes, these are existing flows that I didn't touch 
as it would increase the scope of this PR a lot.
   
   I wanted to keep this PR focused on fixing specific issues, while leaving 
the rest of the flow as it is. Fixing everything in one PR might cause more 
problems. So I didn't touch the rest of the flows in this PR, but added todo 
comments to indicate that I'll look into those in future PRs.



-- 
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]

Reply via email to