Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-11 Thread via GitHub 


cryptoe merged PR #17443:
URL: https://github.com/apache/druid/pull/17443


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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-11 Thread via GitHub 


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


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryKit.java:
##
@@ -54,10 +57,12 @@ public class WindowOperatorQueryKit implements 
QueryKit
 {
   private static final Logger log = new Logger(WindowOperatorQueryKit.class);
   private final ObjectMapper jsonMapper;
+  private final QueryContext queryContext;

Review Comment:
   Makes sense, have made the change.



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-11 Thread via GitHub 


cryptoe commented on code in PR #17443:
URL: https://github.com/apache/druid/pull/17443#discussion_r1836969620


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/WindowOperatorQueryKit.java:
##
@@ -54,10 +57,12 @@ public class WindowOperatorQueryKit implements 
QueryKit
 {
   private static final Logger log = new Logger(WindowOperatorQueryKit.class);
   private final ObjectMapper jsonMapper;
+  private final QueryContext queryContext;

Review Comment:
   I would just store the flag is WindowedOperationTranformEnabled no ?
   There is no need to store the entire context as a class level object. I 
would go one level further and just pass the flag from outside. Ie 
   `new 
WindowOperatorQueryKit(context.jsonMapper(),isGlueingTranformEnabled()))` so 
that queryKits never see the "high level context" and all logic is contained in 
the controller impl. 



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-11 Thread via GitHub 


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


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   > 
https://github.com/implydata/druid/blob/e5be9f13ba674e2f7780dc90ee72e6056429de50/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java#L577
   Why don't we pass it here ? Like let the queryKit have a flag ?
   
   @cryptoe As discussed offline, have made this change.



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


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


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   > Like the most obvious question is how is 
datasource.getQuery().getCOntext() different from queryContext. That nuance is 
hard to reason about when a person is writing an implementation.
   
   @cryptoe Yeah, I fully agree, I was also confused on this.
   
   The problem seems to be that wherever we do 
`query.withOverriddenContext(some context overrides)`, the context overrides 
are applied only to the top-level-query, not to all the nested `Query` objects 
within the query-datasource-query-chains.
   
   I'm not sure if the above is the "only" thing leading to the mismatched 
context maps, but it's definitely one of the reasons.
   
   I'm not sure if it's intentional, but maybe it is? For example, we add a 
`scanSignature` key to ScanQuery object, and it doesn't make sense to pass it 
to all the nested Query objects. So maybe the design originally stemmed from 
such requirements? (Not sure though, I'm lacking the historic context behind 
this design choice)
   
   Thoughts?



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


cryptoe commented on code in PR #17443:
URL: https://github.com/apache/druid/pull/17443#discussion_r1832129015


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   Its confusing. 
   As a developer, dataSource.getQuery() should have all the data that is 
required for me to take a decision. Like the most obvious question is how is 
datasource.getQuery().getCOntext() different from queryContext. That nuance is 
hard to reason about when a person is writing an implementation.  
   



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


cryptoe commented on code in PR #17443:
URL: https://github.com/apache/druid/pull/17443#discussion_r1832129015


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   Its confusing. 
   As a developer, dataSource.getQuery() should have all the data that is 
required for me to take a decision. Like the most obvious question is how is 
datasource.getQuery().getCOntext() is different from queryContext. That nuance 
is hard to reason about when a person is writing an implementation.  
   



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


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


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   I'm not sure why we don't pass query context through the queryKit layers 
directly. We can certainly add a `queryContext` param in 
`QueryKit#makeQueryDefinition` interface method, and it'll help clean up the 
changes of DataSourcePlan layer.
   
   I don't have a strong preference either way on this, happy to make the 
change if you'd prefer that.
   
   (Although on second thoughts, a better way to do it might be to take it as a 
separate work item, and also check if some existing logic of DataSourcePlan 
layer also becomes unnecessary after passing query context directly - not sure 
if that might be the case though)



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


cryptoe commented on code in PR #17443:
URL: https://github.com/apache/druid/pull/17443#discussion_r1832118753


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   
https://github.com/implydata/druid/blob/e5be9f13ba674e2f7780dc90ee72e6056429de50/extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/exec/ControllerImpl.java#L577
   Why don't we pass it here ? Like let the queryKit have a flag ?



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


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


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/MultiStageQueryContext.java:
##
@@ -191,6 +191,8 @@ public class MultiStageQueryContext
 
   public static final String MAX_ROWS_MATERIALIZED_IN_WINDOW = 
"maxRowsMaterializedInWindow";
 
+  public static final String WINDOW_FUNCTION_OPERATOR_TRANSFORMATION = 
"windowFunctionOperatorTransformation";

Review Comment:
   Makes sense, have added.



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


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


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   The changes of this file are essentially to pass the 
`WINDOW_FUNCTION_OPERATOR_TRANSFORMATION` flag to the `WindowOperatorQueryKit` 
layer (as otherwise the flag set in `MSQTaskQueryMaker` layer isn't passed to 
`WindowOperatorQueryKit`).
   
   So, for that, I needed to set the flag in the `query` being passed in 
`queryKitSpec.getQueryKit().makeQueryDefinition()` call in this method.
   
   So I've passed the query context to this method to be able to get the flag's 
value.



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

Re: [PR] MSQ WF: Pass a flag from broker to determine operator chain transformation (druid)

2024-11-06 Thread via GitHub 


cryptoe commented on code in PR #17443:
URL: https://github.com/apache/druid/pull/17443#discussion_r1832101616


##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java:
##
@@ -413,15 +418,25 @@ private static DataSourcePlan forLookup(
   private static DataSourcePlan forQuery(
   final QueryKitSpec queryKitSpec,
   final QueryDataSource dataSource,
+  final QueryContext queryContext,

Review Comment:
   Why is query context required ?



##
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/util/MultiStageQueryContext.java:
##
@@ -191,6 +191,8 @@ public class MultiStageQueryContext
 
   public static final String MAX_ROWS_MATERIALIZED_IN_WINDOW = 
"maxRowsMaterializedInWindow";
 
+  public static final String WINDOW_FUNCTION_OPERATOR_TRANSFORMATION = 
"windowFunctionOperatorTransformation";

Review Comment:
   Could you please add a dev note here mentioning this flag can be removed in 
druid 33 and will be always enabled?



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