Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor fixes and enhancements in UnionQuery handling #17483

Merged
merged 111 commits into from
Nov 28, 2024
Merged
Show file tree
Hide file tree
Changes from 108 commits
Commits
Show all changes
111 commits
Select commit Hold shift + click to select a range
31574b0
annotated enum contextvar
kgyrtkirk Nov 6, 2024
c4d8d6c
fix some stuff
kgyrtkirk Nov 6, 2024
755528b
cleanup
kgyrtkirk Nov 6, 2024
38961d2
add some stuff
kgyrtkirk Nov 6, 2024
9e540a8
crappy fix
kgyrtkirk Nov 6, 2024
433ab05
hacky fix
kgyrtkirk Nov 6, 2024
6baae00
some
kgyrtkirk Nov 6, 2024
39d828f
cleanup
kgyrtkirk Nov 6, 2024
9e21086
make treeset away
kgyrtkirk Nov 6, 2024
8ef3e4c
fx
kgyrtkirk Nov 6, 2024
2ee7005
fix and update1
kgyrtkirk Nov 6, 2024
44c098a
column order changes
kgyrtkirk Nov 6, 2024
579a848
sig changes
kgyrtkirk Nov 6, 2024
c96ef1a
column order in joinqueytest
kgyrtkirk Nov 6, 2024
fbc1bb5
column order in calcitequerytest
kgyrtkirk Nov 6, 2024
9a963c6
fix subq
kgyrtkirk Nov 6, 2024
ce123f8
fix a few more
kgyrtkirk Nov 6, 2024
a68f470
up
kgyrtkirk Nov 6, 2024
cb35cf6
updates
kgyrtkirk Nov 6, 2024
d0b25e5
up
kgyrtkirk Nov 6, 2024
ac0f5ea
fix
kgyrtkirk Nov 6, 2024
ad14eab
update
kgyrtkirk Nov 6, 2024
e67ff79
column order changes
kgyrtkirk Nov 6, 2024
387f0e7
order in join.iq
kgyrtkirk Nov 6, 2024
35c3772
fix test
kgyrtkirk Nov 7, 2024
2711a4f
update test
kgyrtkirk Nov 7, 2024
fd70ad9
fix things in scanquery
kgyrtkirk Nov 7, 2024
fa16e51
add crap and fix calcitequerytest
kgyrtkirk Nov 7, 2024
043470d
fix joinquerytest
kgyrtkirk Nov 7, 2024
2b010b0
fix more
kgyrtkirk Nov 7, 2024
b622aef
fix
kgyrtkirk Nov 7, 2024
ce7aaf7
fix
kgyrtkirk Nov 7, 2024
0bed635
fix
kgyrtkirk Nov 7, 2024
7101c66
update crap
kgyrtkirk Nov 7, 2024
c0f6bd6
fix some/etc
kgyrtkirk Nov 7, 2024
cf8c4ee
fix
kgyrtkirk Nov 7, 2024
590fc01
remove immuatblelist
kgyrtkirk Nov 7, 2024
13a8a26
more
kgyrtkirk Nov 7, 2024
6ce1c0c
cleanupo/fix/etc
kgyrtkirk Nov 7, 2024
b8720b6
fix
kgyrtkirk Nov 7, 2024
63f0a43
make msqa pass
kgyrtkirk Nov 7, 2024
b4037a5
fix
kgyrtkirk Nov 7, 2024
33d57a6
fix onemore
kgyrtkirk Nov 7, 2024
f417893
update comment
kgyrtkirk Nov 8, 2024
9060482
cleanupo/fix/etc
kgyrtkirk Nov 7, 2024
554d244
fix
kgyrtkirk Nov 7, 2024
ebf61ce
make msqa pass
kgyrtkirk Nov 7, 2024
f13812b
fix
kgyrtkirk Nov 7, 2024
f642579
x
kgyrtkirk Nov 7, 2024
fa6c91e
x
kgyrtkirk Nov 7, 2024
44e64c7
x
kgyrtkirk Nov 7, 2024
956c8ef
fix onemore
kgyrtkirk Nov 7, 2024
f9c6d1f
fix onemore
kgyrtkirk Nov 7, 2024
0c82c61
fix onemore
kgyrtkirk Nov 7, 2024
15e4712
fix onemore
kgyrtkirk Nov 7, 2024
89e36dc
fix onemore
kgyrtkirk Nov 7, 2024
0c61433
update comment
kgyrtkirk Nov 8, 2024
436dc35
fix some
kgyrtkirk Nov 8, 2024
bc64245
remove immutablellist
kgyrtkirk Nov 8, 2024
efc97ce
40
kgyrtkirk Nov 8, 2024
42e4e22
fiox
kgyrtkirk Nov 8, 2024
c233381
fix cases
kgyrtkirk Nov 8, 2024
3680fdb
fix
kgyrtkirk Nov 8, 2024
cb474cb
fix
kgyrtkirk Nov 8, 2024
7ccf86a
make some more
kgyrtkirk Nov 8, 2024
22ede9b
sqlcompat done
kgyrtkirk Nov 8, 2024
706f86e
n1
kgyrtkirk Nov 8, 2024
974b489
onemore
kgyrtkirk Nov 8, 2024
1290a90
remove fixing tools
kgyrtkirk Nov 8, 2024
e11048f
add comment
kgyrtkirk Nov 8, 2024
4d2428e
up
kgyrtkirk Nov 8, 2024
108e9e0
clean
kgyrtkirk Nov 8, 2024
62c3228
up
kgyrtkirk Nov 8, 2024
6157fe9
more wood
kgyrtkirk Nov 8, 2024
3426fce
Merge branch 'change-scan-column-order' into decouple-union-fixes
kgyrtkirk Nov 8, 2024
862bab7
add flip/flop test
kgyrtkirk Nov 8, 2024
7021b3f
working?
kgyrtkirk Nov 8, 2024
54b057e
x
kgyrtkirk Nov 8, 2024
c782790
Merge branch 'unnest-relfieldtrimmer' into decouple-union-fixes
kgyrtkirk Nov 8, 2024
b8ed336
up
kgyrtkirk Nov 11, 2024
9af9e45
Revert "up"
kgyrtkirk Nov 11, 2024
b351a64
Revert "remove fixing tools"
kgyrtkirk Nov 11, 2024
d2ae617
coltypes
kgyrtkirk Nov 11, 2024
80925a8
update wx
kgyrtkirk Nov 11, 2024
7ad5c28
remove fixing tools
kgyrtkirk Nov 8, 2024
fc8edf2
Merge branch 'scanquery-fix-equals' into decouple-union-fixes
kgyrtkirk Nov 11, 2024
05c8f15
Merge commit '390c2d68c858ab29388dd56fbbc81847ed204e31' into decouple…
kgyrtkirk Nov 15, 2024
c9b072a
remove relfieldtrimmer for now
kgyrtkirk Nov 15, 2024
0bd6efc
avoid changing behaviour for non-decoupled mode
kgyrtkirk Nov 15, 2024
82ffb42
add comment back
kgyrtkirk Nov 15, 2024
5371004
add comment back
kgyrtkirk Nov 15, 2024
cee074b
amek it so
kgyrtkirk Nov 15, 2024
a30e281
cleanup
kgyrtkirk Nov 15, 2024
c424f26
cleanup
kgyrtkirk Nov 15, 2024
c9420a5
address fixme
kgyrtkirk Nov 15, 2024
353073f
undo
kgyrtkirk Nov 15, 2024
b9116b8
fix style
kgyrtkirk Nov 15, 2024
0a9a30a
Merge remote-tracking branch 'apache/master' into decouple-union-fixes
kgyrtkirk Nov 20, 2024
e3a431d
fix
kgyrtkirk Nov 20, 2024
6c449e7
rename ; remove addition of ctx
kgyrtkirk Nov 20, 2024
bd679e2
correct method name
kgyrtkirk Nov 21, 2024
8f25c51
override
kgyrtkirk Nov 21, 2024
9514c6e
Merge remote-tracking branch 'apache/master' into decouple-union-fixes
kgyrtkirk Nov 22, 2024
67c0416
fix test without refactor
kgyrtkirk Nov 25, 2024
4cf6abd
add
kgyrtkirk Nov 26, 2024
30bb3a8
remove
kgyrtkirk Nov 26, 2024
7a8ff59
add test for iscompat
kgyrtkirk Nov 26, 2024
84eeb18
add test
kgyrtkirk Nov 26, 2024
ea65161
rename test method
kgyrtkirk Nov 27, 2024
dc9bd15
updates
kgyrtkirk Nov 27, 2024
1d68e97
empty
kgyrtkirk Nov 27, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,12 @@ protected QueryTestBuilder testBuilder()
*/
@Test
@Override
public void testUnionIsUnplannable()
public void testUnionIsUnplannableInNative()
Fixed Show fixed Hide fixed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the name change, why does this evaluate native-only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original wording have made me end up with an unfortunate name; renamed it to testUnionDifferentColumnOrder

{
assertQueryIsUnplannable(
"SELECT dim2, dim1, m1 FROM foo2 UNION SELECT dim1, dim2, m1 FROM foo",
"SQL requires union between two tables and column names queried for each table are different Left: [dim2, dim1, m1], Right: [dim1, dim2, m1]."
);

}

@Disabled("Ignored till MSQ can plan UNION ALL with any operand")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -661,4 +661,13 @@ public String toString()
"context=" + context +
'}';
}

public boolean isDecoupledMode()
{
String value = getString(
QueryContexts.CTX_NATIVE_QUERY_SQL_PLANNING_MODE,
QueryContexts.NATIVE_QUERY_SQL_PLANNING_MODE_COUPLED
);
return QueryContexts.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED.equals(value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.druid.java.util.common.StringUtils;

import javax.annotation.Nullable;

import java.math.BigDecimal;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -105,6 +106,10 @@ public class QueryContexts
// SQL statement resource specific keys
public static final String CTX_EXECUTION_MODE = "executionMode";

public static final String CTX_NATIVE_QUERY_SQL_PLANNING_MODE = "plannerStrategy";
public static final String NATIVE_QUERY_SQL_PLANNING_MODE_COUPLED = "COUPLED";
public static final String NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED = "DECOUPLED";

// Defaults
public static final boolean DEFAULT_BY_SEGMENT = false;
public static final boolean DEFAULT_POPULATE_CACHE = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,9 @@ public String toString()
"dataSources=" + dataSources +
'}';
}

public static boolean isCompatibleDataSource(DataSource dataSource)
{
return (dataSource instanceof TableDataSource || dataSource instanceof InlineDataSource);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,13 @@ public boolean isSkipEmptyBuckets()
@Override
public RowSignature getResultRowSignature(Finalization finalization)
{
final Finalization finalization1 = finalization;
final RowSignature.Builder builder = RowSignature.builder();
builder.addTimeColumn();
String timestampResultField = getTimestampResultField();
if (StringUtils.isNotEmpty(timestampResultField)) {
builder.add(timestampResultField, ColumnType.LONG);
}
builder.addAggregators(aggregatorSpecs, finalization1);
builder.addAggregators(aggregatorSpecs, finalization);
builder.addPostAggregators(postAggregatorSpecs);
return builder.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public Sequence<Object> run(QueryPlus<Object> queryPlus, ResponseContext respons
Sequence run = runner.run(queryPlus.withQuery(q), responseContext);
seqs.add(run);
}

return Sequences.concat(seqs);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,20 @@ public void testDefaultEnableQueryDebugging()
assertTrue(QueryContext.of(ImmutableMap.of(QueryContexts.ENABLE_DEBUG, true)).isDebug());
}

@Test
public void testDefaultIsDecoupled()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps also test what happens when a completely different, random value is set. Like "bilyBob"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
assertFalse(QueryContext.empty().isDecoupledMode());
assertTrue(
QueryContext.of(
ImmutableMap.of(
QueryContexts.CTX_NATIVE_QUERY_SQL_PLANNING_MODE,
QueryContexts.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED
)
).isDecoupledMode()
);
}

// This test is a bit silly. It is retained because another test uses the
// LegacyContextQuery test.
@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import com.google.common.collect.ImmutableSet;
import nl.jqno.equalsverifier.EqualsVerifier;
import org.apache.druid.segment.TestHelper;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.join.NoopDataSource;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -62,6 +64,17 @@ public void test_constructor_empty()
new UnionDataSource(Collections.emptyList());
}

@Test
public void test_isCompatible()
{
TableDataSource tableDataSource = new TableDataSource("foo");
InlineDataSource inlineDataSource = InlineDataSource.fromIterable(Collections.emptyList(), RowSignature.empty());

Assert.assertTrue(UnionDataSource.isCompatibleDataSource(tableDataSource));
Assert.assertTrue(UnionDataSource.isCompatibleDataSource(inlineDataSource));
Assert.assertFalse(UnionDataSource.isCompatibleDataSource(new NoopDataSource()));
}

@Test
public void test_getTableNames()
{
Expand Down Expand Up @@ -131,7 +144,7 @@ public void test_withChildren_sameNumber()
//noinspection unchecked
Assert.assertEquals(
new UnionDataSource(newDataSources),
unionDataSource.withChildren((List) newDataSources)
unionDataSource.withChildren(newDataSources)
);
}

Expand Down
9 changes: 4 additions & 5 deletions sql/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,10 @@
<groupId>org.apache.calcite.avatica</groupId>
<artifactId>avatica-metrics</artifactId>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
<dependency>
<groupId>org.checkerframework</groupId>
<artifactId>checker-qual</artifactId>
Expand Down Expand Up @@ -239,11 +243,6 @@
<version>1.3.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>pl.pragmatists</groupId>
<artifactId>JUnitParams</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ public class PlannerConfig
public static final String CTX_KEY_USE_NATIVE_QUERY_EXPLAIN = "useNativeQueryExplain";
public static final String CTX_KEY_FORCE_EXPRESSION_VIRTUAL_COLUMNS = "forceExpressionVirtualColumns";
public static final String CTX_MAX_NUMERIC_IN_FILTERS = "maxNumericInFilters";
public static final String CTX_NATIVE_QUERY_SQL_PLANNING_MODE = "plannerStrategy";
public static final int NUM_FILTER_NOT_USED = -1;

@JsonProperty
private int maxTopNLimit = 100_000;

Expand Down Expand Up @@ -75,9 +73,7 @@ public class PlannerConfig
private int maxNumericInFilters = NUM_FILTER_NOT_USED;

@JsonProperty
private String nativeQuerySqlPlanningMode = NATIVE_QUERY_SQL_PLANNING_MODE_COUPLED; // can be COUPLED or DECOUPLED
public static final String NATIVE_QUERY_SQL_PLANNING_MODE_COUPLED = "COUPLED";
public static final String NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED = "DECOUPLED";
private String nativeQuerySqlPlanningMode = QueryContexts.NATIVE_QUERY_SQL_PLANNING_MODE_COUPLED; // can be COUPLED or DECOUPLED

private boolean serializeComplexValues = true;

Expand Down Expand Up @@ -383,7 +379,7 @@ public Builder withOverrides(final Map<String, Object> queryContext)
maxNumericInFilters);
nativeQuerySqlPlanningMode = QueryContexts.parseString(
queryContext,
CTX_NATIVE_QUERY_SQL_PLANNING_MODE,
QueryContexts.CTX_NATIVE_QUERY_SQL_PLANNING_MODE,
nativeQuerySqlPlanningMode
);
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.calcite.linq4j.QueryProvider;
import org.apache.calcite.schema.SchemaPlus;
import org.apache.druid.common.config.NullHandling;
import org.apache.druid.error.InvalidSqlInput;
import org.apache.druid.java.util.common.DateTimes;
import org.apache.druid.java.util.common.ISE;
import org.apache.druid.java.util.common.Numbers;
Expand Down Expand Up @@ -508,6 +509,9 @@ public String getPlanningError()
*/
public void setPlanningError(String formatText, Object... arguments)
{
if (queryContext().isDecoupledMode()) {
throw InvalidSqlInput.exception(formatText, arguments);
}
Comment on lines +512 to +514
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this whole class should be removed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

queryContext is passed as a Map instead of QueryContext
I'll do this as a separate change

planningError = StringUtils.nonStrictFormat(formatText, arguments);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.apache.calcite.tools.Frameworks;
import org.apache.druid.guice.annotations.Json;
import org.apache.druid.math.expr.ExprMacroTable;
import org.apache.druid.query.QueryContexts;
import org.apache.druid.segment.join.JoinableFactoryWrapper;
import org.apache.druid.server.security.Access;
import org.apache.druid.server.security.AuthConfig;
Expand Down Expand Up @@ -196,7 +197,7 @@ public SqlConformance conformance()
}
});

if (PlannerConfig.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED
if (QueryContexts.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED
.equals(plannerConfig().getNativeQuerySqlPlanningMode())
) {
frameworkConfigBuilder.costFactory(new DruidVolcanoCost.Factory());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.apache.druid.java.util.common.guava.Sequences;
import org.apache.druid.java.util.emitter.EmittingLogger;
import org.apache.druid.query.Query;
import org.apache.druid.query.QueryContexts;
import org.apache.druid.server.QueryResponse;
import org.apache.druid.server.security.Action;
import org.apache.druid.server.security.Resource;
Expand Down Expand Up @@ -540,7 +541,7 @@ protected PlannerResult planWithDruidConvention() throws ValidationException

if (plannerContext.getPlannerConfig()
.getNativeQuerySqlPlanningMode()
.equals(PlannerConfig.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED)
.equals(QueryContexts.NATIVE_QUERY_SQL_PLANNING_MODE_DECOUPLED)
) {
RelNode newRoot = parameterized;
newRoot = planner.transform(
Expand Down
Loading
Loading