alex-plekhanov commented on code in PR #11935:
URL: https://github.com/apache/ignite/pull/11935#discussion_r2027131412
##########
modules/core/src/test/java/org/apache/ignite/testframework/junits/GridAbstractTest.java:
##########
@@ -467,17 +466,26 @@ protected IgniteLogger log() {
}
/**
- * Sets the log level for logger ({@link #log}) to {@link Level#DEBUG}.
The log level will be resetted to
- * default in {@link #afterTest()}.
+ * Sets debug log level for {@link #log}. The log level is resetted to the
default in {@link #afterTest()}.
*/
protected final void setLoggerDebugLevel() {
- String logName = "org.apache.ignite";
+ setLoggerLevel("org.apache.ignite", Level.DEBUG);
+ }
+ /**
+ * Sets the log level for logger ({@link #log}). The log level is resetted
to the default in {@link #afterTest()}.
+ * @return Previous log level.
+ */
+ protected final Level setLoggerLevel(String logName, Level newLvl) {
Review Comment:
This change is not required anymore
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerHelper.java:
##########
@@ -161,6 +188,183 @@ private static void fastenJoinsOrder(IgnitePlanner
planner, RelNode rel) {
planner.addDisabledRules(Arrays.asList(JoinPushThroughJoinRule.LEFT.toString(),
JoinPushThroughJoinRule.RIGHT.toString()));
}
+
+ return rel;
+ }
+
+ /**
+ * Tries to optimize joins order.
+ *
+ * @see JoinToMultiJoinRule
+ * @see IgniteMultiJoinOptimizeRule
+ *
+ * @return An node with optimized joins or original {@code root} if didn't
optimize.
+ */
+ private static RelNode optimizeJoinsOrder(IgnitePlanner planner, RelNode
root, List<RelHint> topLevelHints) {
+ List<Join> joins = findNodes(root, Join.class, false);
+
+ if (joins.isEmpty())
+ return checkJoinsCommutes(planner, root);
+
+ int disabledCnt = 0;
+
+ // If all the joins have the forced order, no need to optimize the
joins order at all.
+ for (RelNode join : joins) {
+ for (RelHint hint : ((Hintable)join).getHints()) {
+ if
(HintDefinition.ENFORCE_JOIN_ORDER.name().equals(hint.hintName)) {
+ ++disabledCnt;
+
+ break;
+ }
+ }
+ }
+
+ if (joins.size() - disabledCnt < JOINS_COUNT_FOR_HEURISTIC_ORDER)
+ return checkJoinsCommutes(planner, root);
+
+ RelNode res = planner.transform(PlannerPhase.HEP_OPTIMIZE_JOIN_ORDER,
root.getTraitSet(), root);
+
+ // Still has a MultiJoin, didn't manage to collect one flat join to
optimize.
+ if (!findNodes(res, MultiJoin.class, true).isEmpty())
+ return checkJoinsCommutes(planner, root);
+
+ // If a new joins order was proposed, no need to launch another join
order optimizations.
+
planner.addDisabledRules(HintDefinition.ENFORCE_JOIN_ORDER.disabledRules().stream().map(RelOptRule::toString)
+ .collect(Collectors.toSet()));
+
+ if (!topLevelHints.isEmpty()) {
+ res = actualTopLevelJoinTypeHints(res, topLevelHints,
joins.get(0));
+
+ restoreJoinTypeHints(res);
+ }
+
+ return res;
+ }
+
+ /** */
+ private static RelNode actualTopLevelJoinTypeHints(RelNode rel,
List<RelHint> topLevelHints, Join filterNode) {
+ assert rel instanceof Hintable;
+
+ // Ignore inheritance to compare hints type and options.
+ List<RelHint> relHints = ((Hintable)rel).getHints().stream()
+ .map(h -> h.inheritPath.isEmpty() ? h :
h.copy(Collections.emptyList())).collect(Collectors.toList());
+
+ List<RelHint> res = new ArrayList<>(topLevelHints.size());
+
+ for (RelHint topHint : topLevelHints) {
+ assert topHint.inheritPath.isEmpty();
+
+ boolean storeHint = true;
+
+ for (RelHint curHint : relHints) {
+ if (topHint.equals(curHint)) {
+ storeHint = false;
+
+ break;
+ }
+ }
+
+ if (storeHint)
+ res.add(topHint);
+ }
+
+ // Keep hints only for joins.
+ res =
Commons.context(filterNode).config().getSqlToRelConverterConfig().getHintStrategyTable().apply(res,
filterNode);
+
+ if (!res.isEmpty()) {
+ rel = ((Hintable)rel).withHints(res);
Review Comment:
Why can't we do just `((Hintable)rel).attachHints(res)`? Looks like original
rel hints should not be changed during `actualTopLevelJoinTypeHints` method,
but now we change it (remove inheritPath).
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/RuleApplyListener.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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.ignite.internal.processors.query.calcite;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
+import org.apache.calcite.plan.RelOptListener;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RuleEventLogger;
+
+/** Watches appliying of a rule. */
+public class RuleApplyListener extends RuleEventLogger {
+ /** */
+ private final RelOptRule rule;
+
+ /** */
+ private final Set<RelOptRuleCall> products = new HashSet<>();
+
+ /** */
+ public final Collection<RelOptListener.RuleAttemptedEvent> tries = new
ArrayList<>();
+
+ /** */
+ public RuleApplyListener(RelOptRule rule) {
+ this.rule = rule;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void ruleAttempted(RuleAttemptedEvent evt) {
+ assert evt.getRuleCall().getRule() == rule ||
!rule.getClass().isAssignableFrom(evt.getRuleCall().getRule().getClass());
+
+ if (evt.getRuleCall().getRule() == rule && !evt.isBefore())
+ tries.add(evt);
+
+ super.ruleAttempted(evt);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void ruleProductionSucceeded(RuleProductionEvent evt) {
+ assert evt.getRuleCall().getRule() == rule ||
!rule.getClass().isAssignableFrom(evt.getRuleCall().getRule().getClass());
+
+ if (evt.getRuleCall().getRule() == rule && !evt.isBefore())
+ products.add(evt.getRuleCall());
+
+ super.ruleProductionSucceeded(evt);
+ }
+
+ /** */
+ public boolean checkAndReset() {
+ boolean res = check();
+
+ tries.clear();
+
+ return res;
+ }
+
+ /** */
+ public boolean check() {
Review Comment:
May be rename to something self-descriptive (to define that rule attmpted
and all attempts succeeded) since in the future we can use this listener to
check other conditions (rule attempted, at least one attempt succeeded, etc)?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/QueryChecker.java:
##########
@@ -453,6 +465,8 @@ public void check() {
assertEqualsCollections(expectedResult, res);
}
+
+ return res;
Review Comment:
Check method not suppose to return result (it can be void or return
boolean). I propose to introduce another method, something like:
```
public QueryChecker withResultChecker(Consumer<List<List<?>>>...
resultChecker)
```
Which can consume result set (and store it if needed)
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/rules/JoinOrderOptimizationTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * 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.ignite.internal.processors.query.calcite.rules;
+
+import java.util.Collection;
+import java.util.List;
+import org.apache.calcite.rel.rules.JoinToMultiJoinRule;
+import org.apache.ignite.internal.processors.query.calcite.QueryChecker;
+import org.apache.ignite.internal.processors.query.calcite.RuleApplyListener;
+import
org.apache.ignite.internal.processors.query.calcite.integration.AbstractBasicIntegrationTest;
+import
org.apache.ignite.internal.processors.query.calcite.rule.logical.IgniteMultiJoinOptimizeRule;
+import org.apache.ignite.internal.util.typedef.F;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import static
org.apache.ignite.internal.processors.query.calcite.hint.HintDefinition.ENFORCE_JOIN_ORDER;
+
+/**
+ * Test of joins order heuristic optimization.
+ *
+ * @see JoinToMultiJoinRule
+ * @see IgniteMultiJoinOptimizeRule
+ */
+@RunWith(Parameterized.class)
+public class JoinOrderOptimizationTest extends AbstractBasicIntegrationTest {
+ /** Test query. */
+ @Parameterized.Parameter
+ public String qry;
+
+ /** Test queries. */
+ @Parameterized.Parameters
+ public static Collection<String> runConfig() {
+ return testQueries();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected void beforeTestsStarted() throws Exception {
+ super.beforeTestsStarted();
+
+ initSchema();
+
+ gatherStatistics();
+ }
+
+ /** {@inheritDoc} */
+ @Override protected boolean destroyCachesAfterTest() {
+ return false;
+ }
+
+ /** */
+ private void initSchema() {
+ int warehsSz = 50;
+ int catgSz = 100;
+ int reviewSz = 50000;
+ int prodSz = 100;
+ int discSz = 2000;
+ int shipSz = 15000;
+ int usrSz = 10000;
+ int ordSz = 20000;
+ int ordDetSz = 100000;
+
+ sql("CREATE TABLE Products (ProdId INT PRIMARY KEY, ProdNm
VARCHAR(100), Price DECIMAL(10, 2)) WITH \"VALUE_TYPE='PROD'\"");
+ sql("INSERT INTO Products SELECT x, 'Product_' || x::VARCHAR, 100.0 +
x % 100.0 FROM system_range(1, ?)",
+ prodSz);
+
+ sql("CREATE TABLE ProductCategories (ProdCatgId INT PRIMARY KEY,
ProdId INT, CatgId INT)");
+ sql("INSERT INTO ProductCategories SELECT x, 1 + RAND_INTEGER(?), 1 +
RAND_INTEGER(?) FROM system_range(1, ?)",
+ prodSz - 1, catgSz - 1, prodSz);
+
+ sql("CREATE TABLE Discounts (DiscId INT PRIMARY KEY, ProdId INT,
DiscPercent DECIMAL(5, 2), ValidUntil DATE)");
+ sql("INSERT INTO Discounts SELECT x, 1 + RAND_INTEGER(?), 1 + x % 15,
date '2025-02-10' + (x % 365)::INTERVAL DAYS " +
+ "FROM system_range(1, ?)", prodSz - 1, discSz);
+
+ sql("CREATE TABLE Shipping (ShippId INT PRIMARY KEY, OrdId INT,
ShippDate DATE, ShippAddrs VARCHAR(255))");
+ sql("INSERT INTO Shipping SELECT x, 1 + RAND_INTEGER(?), date
'2020-01-01' + RAND_INTEGER(365)::INTERVAL DAYS, "
+ + " 'Addrs_' || x::VARCHAR FROM system_range(1, ?)", ordSz - 1,
shipSz);
+
+ sql("CREATE TABLE Users (UsrId INT PRIMARY KEY, UsrNm VARCHAR(100),
Email VARCHAR(100)) WITH \"VALUE_TYPE='USR'\"");
+ sql("INSERT INTO Users SELECT x, 'User_' || x::VARCHAR, 'email_' ||
x::VARCHAR || '@nowhere.xyz' FROM system_range(1, ?)",
+ usrSz);
+
+ sql("CREATE TABLE Orders (OrdId INT PRIMARY KEY, UsrId INT, ProdId
INT, OrdDate DATE, TotalAmount DECIMAL(10, 2)) " +
+ "WITH \"VALUE_TYPE='ORD'\"");
+ sql("INSERT INTO Orders SELECT x, 1 + RAND_INTEGER(?), 1 +
RAND_INTEGER(?), date '2025-02-10' + (x % 365)::INTERVAL DAYS, " +
+ "1 + x % 10 FROM system_range(1, ?)", usrSz - 1, prodSz, ordSz);
+
+ sql("CREATE TABLE OrderDetails (OrdDetId INT PRIMARY KEY, OrdId INT,
ProdId INT, Qnty INT) WITH \"VALUE_TYPE='ORD_DET'\"");
+ sql("INSERT INTO OrderDetails SELECT x, 1 + RAND_INTEGER(?), 1 +
RAND_INTEGER(?), 1 + x % 10 FROM system_range(1, ?)",
+ ordSz - 1, prodSz - 1, ordDetSz);
+
+ sql("CREATE TABLE Warehouses (WrhId INT PRIMARY KEY, WrhNm
VARCHAR(100), LocNm VARCHAR(100))");
+ sql("INSERT INTO Warehouses SELECT x, 'Wrh_' || x::VARCHAR,
'Location_' || x::VARCHAR FROM system_range(1, ?)", warehsSz);
+
+ sql("CREATE TABLE Categories (CatgId INT PRIMARY KEY, CatgName
VARCHAR(100))");
+ sql("INSERT INTO Categories SELECT x, 'Category_' || x::VARCHAR FROM
system_range(1, ?)", catgSz);
+
+ sql("CREATE TABLE Reviews (RevId INT PRIMARY KEY, ProdId INT, usrId
INT, RevTxt VARCHAR, Rating INT)");
+ sql("INSERT INTO Reviews SELECT x, 1 + RAND_INTEGER(?), 1 +
RAND_INTEGER(?), 'Prod. review ' || x::VARCHAR, 1 + RAND_INTEGER(4) " +
+ "FROM system_range(1, ?)", prodSz - 1, usrSz - 1, reviewSz);
+ }
+
+ /** Tests that query result doesn't change with the joins order
optimization. */
+ @Test
+ public void testTheSameResults() {
+ assert !qry.contains(ENFORCE_JOIN_ORDER.name());
+ assert qry.startsWith("SELECT ");
+
+ String qryFixedJoins = qry.replaceAll("SELECT", "SELECT /*+ " +
ENFORCE_JOIN_ORDER + " */");
+
+ RuleApplyListener planLsnr = new
RuleApplyListener(IgniteMultiJoinOptimizeRule.INSTANCE);
+
+ // First, call with fixed join order.
+ List<List<?>> expectedResult =
assertQuery(qryFixedJoins).withPlannerListener(planLsnr).check();
+
+ // Ensure that the optimization rule wasn't fired.
+ assertFalse(planLsnr.check());
Review Comment:
checkAndReset?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/AbstractPlannerTest.java:
##########
@@ -222,13 +224,46 @@ public static <T extends RelNode> Predicate<RelNode>
byClass(Class<T> cls, Predi
/** */
protected PlanningContext plannerCtx(String sql, IgniteSchema
publicSchema, String... disabledRules) {
- return plannerCtx(sql, Collections.singleton(publicSchema),
disabledRules);
+ return plannerCtx(sql, publicSchema, null, disabledRules);
}
/** */
- protected PlanningContext plannerCtx(String sql, Collection<IgniteSchema>
schemas, String... disabledRules) {
+ protected PlanningContext plannerCtx(
+ String sql,
+ IgniteSchema publicSchema,
+ @Nullable RelOptListener planLsnr,
+ String... disabledRules
+ ) {
+ return plannerCtx(sql, Collections.singleton(publicSchema), planLsnr,
disabledRules);
+ }
+
+ /** */
+ protected PlanningContext plannerCtx(
+ String sql,
+ Collection<IgniteSchema> schemas,
+ @Nullable RelOptListener planLsnr,
+ String... disabledRules
+ ) {
+ final Context baseCtx0 = baseQueryContext(schemas);
+ final Context baseCtx;
+
+ if (planLsnr != null) {
+ baseCtx = new Context() {
+ @Override public <C>
@org.checkerframework.checker.nullness.qual.Nullable C unwrap(Class<C> aCls) {
+ C res = baseCtx0.unwrap(aCls);
+
+ if (res == null &&
RelOptListener.class.isAssignableFrom(aCls) && planLsnr != null)
+ return (C)planLsnr;
+
+ return res;
+ }
+ };
+ }
+ else
+ baseCtx = baseCtx0;
+
PlanningContext ctx = PlanningContext.builder()
- .parentContext(baseQueryContext(schemas))
+ .parentContext(baseCtx)
Review Comment:
Just:
```
.parentContext(Contexts.of(baseQueryContext(schemas), planLsnr))
```
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/RuleApplyListener.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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.ignite.internal.processors.query.calcite;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
+import org.apache.calcite.plan.RelOptListener;
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RuleEventLogger;
+
+/** Watches appliying of a rule. */
+public class RuleApplyListener extends RuleEventLogger {
+ /** */
+ private final RelOptRule rule;
+
+ /** */
+ private final Set<RelOptRuleCall> products = new HashSet<>();
+
+ /** */
+ public final Collection<RelOptListener.RuleAttemptedEvent> tries = new
ArrayList<>();
+
+ /** */
+ public RuleApplyListener(RelOptRule rule) {
+ this.rule = rule;
+ }
+
+ /** {@inheritDoc} */
+ @Override public void ruleAttempted(RuleAttemptedEvent evt) {
+ assert evt.getRuleCall().getRule() == rule ||
!rule.getClass().isAssignableFrom(evt.getRuleCall().getRule().getClass());
+
+ if (evt.getRuleCall().getRule() == rule && !evt.isBefore())
+ tries.add(evt);
+
+ super.ruleAttempted(evt);
+ }
+
+ /** {@inheritDoc} */
+ @Override public void ruleProductionSucceeded(RuleProductionEvent evt) {
+ assert evt.getRuleCall().getRule() == rule ||
!rule.getClass().isAssignableFrom(evt.getRuleCall().getRule().getClass());
+
+ if (evt.getRuleCall().getRule() == rule && !evt.isBefore())
+ products.add(evt.getRuleCall());
+
+ super.ruleProductionSucceeded(evt);
+ }
+
+ /** */
+ public boolean checkAndReset() {
+ boolean res = check();
+
+ tries.clear();
+
Review Comment:
`products.clear()`?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
