LakshSingla commented on code in PR #14981:
URL: https://github.com/apache/druid/pull/14981#discussion_r1326472305
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java:
##########
@@ -356,4 +359,44 @@ public void testTooManyInputFiles() throws IOException
.setExpectedMSQFault(new TooManyInputFilesFault(numFiles,
Limits.MAX_INPUT_FILES_PER_WORKER, 2))
.verifyResults();
}
+
+ @Test
+ public void testUnionAllIsDisallowed()
+ {
+ final RowSignature rowSignature =
+ RowSignature.builder().add("__time", ColumnType.LONG).build();
+ testIngestQuery()
+ .setSql("SELECT * FROM foo\n"
+ + "UNION ALL\n"
+ + "SELECT * FROM foo\n")
+ .setExpectedRowSignature(rowSignature)
+ .setExpectedDataSource("foo1")
+ .setExpectedMSQFault(QueryNotSupportedFault.instance())
Review Comment:
It does plan using the `UnionDataSource`, which then goes into MSQ.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java:
##########
@@ -356,4 +359,44 @@ public void testTooManyInputFiles() throws IOException
.setExpectedMSQFault(new TooManyInputFilesFault(numFiles,
Limits.MAX_INPUT_FILES_PER_WORKER, 2))
.verifyResults();
}
+
+ @Test
+ public void testUnionAllIsDisallowed()
+ {
+ final RowSignature rowSignature =
+ RowSignature.builder().add("__time", ColumnType.LONG).build();
+ testIngestQuery()
+ .setSql("SELECT * FROM foo\n"
+ + "UNION ALL\n"
+ + "SELECT * FROM foo\n")
+ .setExpectedRowSignature(rowSignature)
+ .setExpectedDataSource("foo1")
+ .setExpectedMSQFault(QueryNotSupportedFault.instance())
+ .verifyResults();
+ }
+
+ @Test
+ public void testUnionAllIsDisallowedWhilePlanning()
+ {
+ // This results in a planning error however the planning error isn't an
accurate representation of the actual error
+ // because Calcite rewrites it using CoreRules.UNION_TO_DISTINCT, which
plans it using Union Datasource.
+ // However, this fails since the column names mismatch. Once MSQ is able
to support Union datasources, the planning
+ // error would become an accurate representation of the error.
+ testIngestQuery()
+ .setSql(
+ "INSERT INTO druid.dst "
+ + "SELECT dim2, dim1, m1 FROM foo2 "
+ + "UNION ALL "
+ + "SELECT dim1, dim2, m1 FROM foo "
+ + "PARTITIONED BY ALL TIME")
+ .setExpectedValidationErrorMatcher(
+ new DruidExceptionMatcher(
+ DruidException.Persona.ADMIN,
+ DruidException.Category.INVALID_INPUT,
+ "general"
+ ).expectMessageIs("Query planning failed for unknown reason, our
best guess is this "
Review Comment:
It's because top-level union all is disallowed, so it tries to plan the
query using the
[UnionDataSourceRule](https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidUnionDataSourceRule.java),
goes into the `isCompatible` then, and then rewrites the already set planning
error.
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidUnionRule.java:
##########
@@ -51,6 +52,13 @@ public DruidUnionRule(PlannerContext plannerContext)
@Override
public boolean matches(RelOptRuleCall call)
{
+ if
(!plannerContext.featureAvailable(EngineFeature.ALLOW_TOP_LEVEL_UNION_ALL)) {
Review Comment:
I was wondering if it's worth it for the cleaner exception message. I'll
conditionally add the rule instead, since eventually, we'll be able to plan the
query using union data source.
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQFaultsTest.java:
##########
@@ -356,4 +359,44 @@ public void testTooManyInputFiles() throws IOException
.setExpectedMSQFault(new TooManyInputFilesFault(numFiles,
Limits.MAX_INPUT_FILES_PER_WORKER, 2))
.verifyResults();
}
+
+ @Test
+ public void testUnionAllIsDisallowed()
+ {
+ final RowSignature rowSignature =
+ RowSignature.builder().add("__time", ColumnType.LONG).build();
+ testIngestQuery()
+ .setSql("SELECT * FROM foo\n"
+ + "UNION ALL\n"
+ + "SELECT * FROM foo\n")
+ .setExpectedRowSignature(rowSignature)
+ .setExpectedDataSource("foo1")
+ .setExpectedMSQFault(QueryNotSupportedFault.instance())
+ .verifyResults();
+ }
+
+ @Test
+ public void testUnionAllIsDisallowedWhilePlanning()
+ {
+ // This results in a planning error however the planning error isn't an
accurate representation of the actual error
+ // because Calcite rewrites it using CoreRules.UNION_TO_DISTINCT, which
plans it using Union Datasource.
Review Comment:
Stale comment, apologies for the confusion. Will clean it up.
--
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]
