gianm commented on code in PR #14981:
URL: https://github.com/apache/druid/pull/14981#discussion_r1326330549
##########
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()
Review Comment:
This isn't an ingest query, why use `testIngestQuery`?
##########
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:
Hmm… I am confused about why this yields a `QueryNotSupportedFault`.
Shouldn't it fail to plan, and generate a planner error instead of an MSQ fault?
##########
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:
Where is this error coming from? Looking at the code for the union rule, I
would think it can't happen, because it's generated by `isCompatible`, which
isn't called when the `ALLOW_TOP_LEVEL_UNION_ALL` feature is missing. The error
should be something about `UNION ALL` being unsupported for this engine.
##########
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:
Hmm… I am confused about why this yields a `QueryNotSupportedFault`.
Shouldn't it fail to plan, and generate a planner error instead of an MSQ fault?
##########
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:
Better to simply not add the rule if this feature is missing. Try adding a
conditional in `DruidRules.rules`. There are others there for other engine
features.
##########
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:
I think that `CoreRules.UNION_TO_DISTINCT` won't fire here, because this is
`UNION ALL`, not `UNION`, and that rule only fires for non-all `UNION`. Is this
comment correct?
--
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]
