You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:21:44 UTC
[GitHub] [druid] suneet-s opened a new pull request #9923: Add parameterized Calcite tests for join queries
suneet-s opened a new pull request #9923:
URL: https://github.com/apache/druid/pull/9923
This change paramterizes the join CalciteQueryTest tests so that we can test the various config options for join queries.
For join queries, we use the query context to enable and disable different features on the query execution path. The results of a query should be the same regardless of which features it uses. These parameterized tests allow us to validate that the results are in fact the same regardless of the passed in query context features.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh merged pull request #9923: Add parameterized Calcite tests for join queries
Posted by GitBox <gi...@apache.org>.
ccaominh merged pull request #9923:
URL: https://github.com/apache/druid/pull/9923
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #9923: Add parameterized Calcite tests for join queries
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #9923:
URL: https://github.com/apache/druid/pull/9923#discussion_r430768726
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -8803,6 +8841,7 @@ public void testInnerJoinTwoLookupsToTableUsingNumericColumn() throws Exception
+ "FROM foo\n"
+ "INNER JOIN lookup.lookyloo l1 ON l1.k = foo.m1\n"
+ "INNER JOIN lookup.lookyloo l2 ON l2.k = l1.k",
+ queryContext,
Review comment:
I left it as is to see if the test validates the query context that is used.
It looks like this is done automatically in `BaseCalciteQueryTest#testQuery` ~ line 619
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -9152,6 +9198,7 @@ public void testCommaJoinTableLookupTableMismatchedTypes() throws Exception
"SELECT COUNT(*)\n"
+ "FROM foo, lookup.lookyloo l, numfoo\n"
+ "WHERE foo.cnt = l.k AND l.k = numfoo.cnt\n",
+ queryContext,
Review comment:
Same as above - when I made these changes I didn't actually dig in to how this was being verified. It's pretty inconsistent across the tests. Do you think it's ok if we remove references to the query context in another PR?
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -9279,22 +9330,24 @@ public void testInnerJoinCastLeft() throws Exception
)
.intervals(querySegmentSpec(Filtration.eternity()))
.columns("j0.k", "j0.v", "m1")
- .context(QUERY_CONTEXT_DEFAULT)
+ .context(queryContext)
Review comment:
Done
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13917,4 +14073,38 @@ public void testRepeatedIdenticalVirtualExpressionGrouping() throws Exception
)
);
}
+
+ /**
+ * This is a provider of query contexts that should be used by join tests.
+ * It tests various configs that can be passed to join queries. All the configs provided by this provider should
+ * have the join query engine return the same results.
+ */
+ public static class QueryContextForJoinProvider
+ {
+ @UsedByJUnitParamsRunner
+ public static Object[] provideQueryContexts()
+ {
+ return new Object[] {
+ // default behavior
+ QUERY_CONTEXT_DEFAULT,
+ // filter value re-writes enabled
+ new ImmutableMap.Builder<String, Object>()
+ .putAll(QUERY_CONTEXT_DEFAULT)
+ .put(QueryContexts.JOIN_FILTER_REWRITE_VALUE_COLUMN_FILTERS_ENABLE_KEY, true)
+ .build(),
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] ccaominh commented on a change in pull request #9923: Add parameterized Calcite tests for join queries
Posted by GitBox <gi...@apache.org>.
ccaominh commented on a change in pull request #9923:
URL: https://github.com/apache/druid/pull/9923#discussion_r430742917
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -13917,4 +14073,38 @@ public void testRepeatedIdenticalVirtualExpressionGrouping() throws Exception
)
);
}
+
+ /**
+ * This is a provider of query contexts that should be used by join tests.
+ * It tests various configs that can be passed to join queries. All the configs provided by this provider should
+ * have the join query engine return the same results.
+ */
+ public static class QueryContextForJoinProvider
+ {
+ @UsedByJUnitParamsRunner
+ public static Object[] provideQueryContexts()
+ {
+ return new Object[] {
+ // default behavior
+ QUERY_CONTEXT_DEFAULT,
+ // filter value re-writes enabled
+ new ImmutableMap.Builder<String, Object>()
+ .putAll(QUERY_CONTEXT_DEFAULT)
+ .put(QueryContexts.JOIN_FILTER_REWRITE_VALUE_COLUMN_FILTERS_ENABLE_KEY, true)
+ .build(),
Review comment:
May be worth explicitly setting JOIN_FILTER_REWRITE_ENABLE_KEY to true here in case the default value changes in the future.
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -8803,6 +8841,7 @@ public void testInnerJoinTwoLookupsToTableUsingNumericColumn() throws Exception
+ "FROM foo\n"
+ "INNER JOIN lookup.lookyloo l1 ON l1.k = foo.m1\n"
+ "INNER JOIN lookup.lookyloo l2 ON l2.k = l1.k",
+ queryContext,
Review comment:
Should line 8863 use `queryContext` instead of `QUERY_CONTEXT_DEFAULT`?
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -9279,22 +9330,24 @@ public void testInnerJoinCastLeft() throws Exception
)
.intervals(querySegmentSpec(Filtration.eternity()))
.columns("j0.k", "j0.v", "m1")
- .context(QUERY_CONTEXT_DEFAULT)
+ .context(queryContext)
Review comment:
Revert extra indent
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -9152,6 +9198,7 @@ public void testCommaJoinTableLookupTableMismatchedTypes() throws Exception
"SELECT COUNT(*)\n"
+ "FROM foo, lookup.lookyloo l, numfoo\n"
+ "WHERE foo.cnt = l.k AND l.k = numfoo.cnt\n",
+ queryContext,
Review comment:
Should line 9219 use queryContext instead of QUERY_CONTEXT_DEFAULT?
##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -9152,6 +9198,7 @@ public void testCommaJoinTableLookupTableMismatchedTypes() throws Exception
"SELECT COUNT(*)\n"
+ "FROM foo, lookup.lookyloo l, numfoo\n"
+ "WHERE foo.cnt = l.k AND l.k = numfoo.cnt\n",
+ queryContext,
Review comment:
Yeah, doesn't need to be addressed by this PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org