You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "gianm (via GitHub)" <gi...@apache.org> on 2023/04/18 03:23:48 UTC
[GitHub] [druid] gianm opened a new pull request, #14105: MSQ: Subclass CalciteJoinQueryTest, other supporting changes.
gianm opened a new pull request, #14105:
URL: https://github.com/apache/druid/pull/14105
The main change is the new tests: we now subclass CalciteJoinQueryTest in CalciteSelectJoinQueryMSQTest twice, once for Broadcast and once for SortMerge.
Two supporting production changes for default-value mode:
1) InputNumberDataSource is marked as concrete, to allow leftFilter to
be pushed down to it.
2) In default-value mode, numeric frame field readers can now return nulls.
This is necessary when stacking joins on top of joins: nulls must be
preserved for semantics that match broadcast joins and native queries.
3) In default-value mode, StringFieldReader.isNull returns true on empty
strings in addition to nulls. This is more consistent with the behavior
of the selectors, which map empty strings to null as well in that mode.
As an effect of change (2), the InsertTimeNull change from #14020 (to replace null timestamps with default timestamps) is reverted. IMO, this is fine, as either behavior is defensible, and the change from #14020 hasn't been released yet.
--
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: commits-unsubscribe@druid.apache.org
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] gianm merged pull request #14105: MSQ: Subclass CalciteJoinQueryTest, other supporting changes.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #14105:
URL: https://github.com/apache/druid/pull/14105
--
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: commits-unsubscribe@druid.apache.org
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] gianm commented on a diff in pull request #14105: MSQ: Subclass CalciteJoinQueryTest, other supporting changes.
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14105:
URL: https://github.com/apache/druid/pull/14105#discussion_r1176930515
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java:
##########
@@ -640,28 +660,28 @@ public void testFilterAndGroupByLookupUsingJoinOperatorAllowNulls(Map<String, Ob
testQuery(
"SELECT lookyloo.v, COUNT(*)\n"
- + "FROM foo LEFT JOIN lookup.lookyloo ON foo.dim2 = lookyloo.k\n"
- + "WHERE lookyloo.v <> 'xa' OR lookyloo.v IS NULL\n"
- + "GROUP BY lookyloo.v",
+ + "FROM foo LEFT JOIN lookup.lookyloo ON foo.dim2 = lookyloo.k\n"
Review Comment:
Ah, yeah, that was the result of applying formatting in IntelliJ. I guess the last time this file was updated, the formatting wasn't applied. To help with reading the diff, Github has this "hide whitespace" feature for PR diffs. If you turn that on it looks like this:
https://github.com/apache/druid/pull/14105/files/ec47a89a785eaf5144aaf9db4752b29e002457c0?w=1#diff-4d1d101fd375322e3a765ae6aa738283f84967d508cf8afccf6f1572af2cd1c7
The substantive changes are indeed adding various `notMsqCompatible` and `sortIfSortBased`.
--
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: commits-unsubscribe@druid.apache.org
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] cryptoe commented on a diff in pull request #14105: MSQ: Subclass CalciteJoinQueryTest, other supporting changes.
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #14105:
URL: https://github.com/apache/druid/pull/14105#discussion_r1176216429
##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/CalciteSelectJoinQueryMSQTest.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.msq.test;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import org.apache.calcite.rel.RelRoot;
+import org.apache.calcite.tools.ValidationException;
+import org.apache.druid.guice.DruidInjectorBuilder;
+import org.apache.druid.msq.exec.WorkerMemoryParameters;
+import org.apache.druid.msq.sql.MSQTaskSqlEngine;
+import org.apache.druid.query.groupby.TestGroupByBuffers;
+import org.apache.druid.server.QueryLifecycleFactory;
+import org.apache.druid.sql.calcite.CalciteJoinQueryTest;
+import org.apache.druid.sql.calcite.QueryTestBuilder;
+import org.apache.druid.sql.calcite.planner.JoinAlgorithm;
+import org.apache.druid.sql.calcite.planner.PlannerContext;
+import org.apache.druid.sql.calcite.run.EngineFeature;
+import org.apache.druid.sql.calcite.run.QueryMaker;
+import org.apache.druid.sql.calcite.run.SqlEngine;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+/**
+ * Runs {@link CalciteJoinQueryTest} but with MSQ engine.
+ */
+@RunWith(Enclosed.class)
Review Comment:
Nice trick
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java:
##########
@@ -640,28 +660,28 @@ public void testFilterAndGroupByLookupUsingJoinOperatorAllowNulls(Map<String, Ob
testQuery(
"SELECT lookyloo.v, COUNT(*)\n"
- + "FROM foo LEFT JOIN lookup.lookyloo ON foo.dim2 = lookyloo.k\n"
- + "WHERE lookyloo.v <> 'xa' OR lookyloo.v IS NULL\n"
- + "GROUP BY lookyloo.v",
+ + "FROM foo LEFT JOIN lookup.lookyloo ON foo.dim2 = lookyloo.k\n"
Review Comment:
There are lot of formatting changes hence its very easy to skip the actual change.
* Couple of tests are marked notMsqCompatible();
* sortIfSortBased result validation.
Are there more changes in this class?
--
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: commits-unsubscribe@druid.apache.org
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