cryptoe commented on code in PR #14886:
URL: https://github.com/apache/druid/pull/14886#discussion_r1314968583
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessor.java:
##########
@@ -96,7 +98,11 @@ private static Pair<List<ReadableFrameChannel>,
BroadcastJoinHelper> makeInputCh
final long memoryReservedForBroadcastJoin
)
{
- if (!(dataSource instanceof JoinDataSource) && !sideChannels.isEmpty()) {
+ // An UnnestDataSource or FilteredDataSource can have a join as a base
+ // In such a case a side channel is expected to be there
+ if (!(dataSource instanceof JoinDataSource
+ || dataSource instanceof UnnestDataSource
Review Comment:
Unnest seems to have 2 data sources FilteredDS and UnnestDS. Could you
please help me understand a bit more here.
Seems like the filter could have easily been pushed to the unnest data
source. Is there are reason not to ?
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidRules.java:
##########
@@ -115,13 +115,13 @@ public static List<RelOptRule> rules(PlannerContext
plannerContext)
retVal.add(DruidOuterQueryRule.WINDOW);
}
- if (plannerContext.featureAvailable(EngineFeature.UNNEST)) {
Review Comment:
nit: I think we should let this flag remain. If for what ever reason,
something doesn't work out, we can just revert the changes in the engines.
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java:
##########
@@ -3052,6 +3066,7 @@ public void
testUnnestThriceWithFiltersOnDimAndUnnestColumnsORCombinations()
{
cannotVectorize();
skipVectorize();
+ notMsqCompatible();
Review Comment:
Lets add the above comment regarding the datasource `lotsofcolumns`in the UT
itself so that we know how to enable it.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQSelectTest.java:
##########
@@ -2119,6 +2121,207 @@ public void testJoinUsesDifferentAlgorithm()
.verifyResults();
}
+ @Test
+ public void testSelectUnnestOnInlineFoo()
Review Comment:
Please add tests cases for insert and replace q's as well.
They would be present in
MSQInsertTests, MSQReplaceTests.
I wonder if we should have some tests in which take in a select query and
the framework makesthe insert and the replace query from it.
--
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]
