Minor fixes and enhancements in UnionQuery handling #17483
Conversation
github-actions
bot
added
Area - Batch Ingestion
Area - MSQ
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteUnionQueryMSQTest.java
Fixed
Show fixed
Hide fixed
| @@ -62,13 +62,12 @@ protected QueryTestBuilder testBuilder() | |||
| */ | |||
| @Test | |||
| @Override | |||
| public void testUnionIsUnplannable() | |||
| public void testUnionIsUnplannableInNative() | |||
There was a problem hiding this comment.
Aside from the name change, why does this evaluate native-only?
There was a problem hiding this comment.
the original wording have made me end up with an unfortunate name; renamed it to testUnionDifferentColumnOrder
| @@ -372,6 +372,20 @@ public void testDefaultEnableQueryDebugging() | |||
| assertTrue(QueryContext.of(ImmutableMap.of(QueryContexts.ENABLE_DEBUG, true)).isDebug()); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testDefaultIsDecoupled() | |||
There was a problem hiding this comment.
Perhaps also test what happens when a completely different, random value is set. Like "bilyBob"
There was a problem hiding this comment.
update the testcase:
- renamed it to
testIsDecoupled - added check for when its set to
garbage
I'm not sure what are the gains of having this as a string based setting;
I don't think it will have more variations...
it could be a boolean with the name like decoupledMode
| if (queryContext().isDecoupledMode()) { | ||
| throw InvalidSqlInput.exception(formatText, arguments); | ||
| } |
There was a problem hiding this comment.
Can we not have a "DecoupledPlannerContext" or something that just overrides this method and throws the exception instead of checks the context? I don't actually know where the PlannerContext is created, so no clue how hard/easy it is to do it, but this check feels ugly.
There was a problem hiding this comment.
I feel like this whole class should be removed
There was a problem hiding this comment.
queryContext is passed as a Map instead of QueryContext
I'll do this as a separate change
| @@ -53,6 +53,9 @@ public DruidUnion( | |||
| boolean all) | |||
| { | |||
| super(cluster, traits, hints, inputs, all); | |||
| if (!all) { | |||
| throw InvalidInput.exception("SQL requires 'UNION' but only 'UNION ALL' is supported."); | |||
There was a problem hiding this comment.
Nit: InvalidSqlInput
kgyrtkirk
changed the title
Fixes some union related issues
UnionDataSourceorUnionQueryfor decoupled modePlannerConfigtoQueryContexts