alex-plekhanov commented on code in PR #10902:
URL: https://github.com/apache/ignite/pull/10902#discussion_r1324579001
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlannerHelper.java:
##########
@@ -113,6 +122,29 @@ public static IgniteRel optimize(SqlNode sqlNode,
IgnitePlanner planner, IgniteL
}
}
+ /**
+ * Extracts planner-level hints like 'DISABLE_RULE' if the root node is a
combining node like 'UNION'.
+ */
+ private static Collection<RelHint> extractRootHints(RelRoot rootRel) {
+ if (rootRel.hints.isEmpty()) {
+ if (!Hint.allRelHints(rootRel.rel).isEmpty())
+ return Hint.allRelHints(rootRel.rel);
+
+ if (rootRel.rel instanceof SetOp) {
+ Collection<RelHint> hints1 =
extractRootHints(rootRel.withRel(rootRel.rel.getInput(0)));
Review Comment:
I think we can create `extractRootHints(RelNode node)` method to avoid
creation of `RelRoot`.
Also, AFAIK SetOp can contain more than two inputs.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/SortAggregateConverterRule.java:
##########
@@ -106,7 +105,7 @@ private static class MapReduceSortAggregateConverterRule
extends AbstractIgniteC
if (F.isEmpty(agg.getGroupSet()) || agg.getGroupSets().size() > 1)
return null;
- if (HintUtils.isExpandDistinctAggregate(agg))
+ if (HashAggregateConverterRule.isExpandedDistinct(agg))
Review Comment:
`HintUtils.isExpandDistinctAggregate(agg)` looks better here in my opinion.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/hint/Hint.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.hint;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.hint.HintStrategyTable;
+import org.apache.calcite.rel.hint.Hintable;
+import org.apache.calcite.rel.hint.RelHint;
+import org.apache.ignite.internal.util.typedef.F;
+
+/**
+ * Base class for working with Calcite's SQL hints.
+ */
+public final class Hint {
Review Comment:
Class name is confusing, it sounds like some wrapper over RelHint, but
actually it contains only static methods, so it's better to use name like
`HintUtils`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/logical/IgniteLogicalIndexScan.java:
##########
@@ -32,6 +31,9 @@
/** */
public class IgniteLogicalIndexScan extends AbstractIndexScan {
+ /** If {@code true}, should remove other index scans over this table of
table scans. */
+ private boolean forced;
Review Comment:
Field `forced` is set only inside `ExposeIndexRule` and used only by
`ExposeIndexRule` (both during one method call), I think it's better to use
some `ExposeIndexRule` local context, than add new field to
`IgniteLogicalIndexScan`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/agg/IgniteColocatedHashAggregate.java:
##########
@@ -18,7 +18,6 @@
package org.apache.ignite.internal.processors.query.calcite.rel.agg;
import java.util.List;
-
Review Comment:
Redundant change
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/hint/HintDefinition.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.hint;
+
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.hint.HintOptionChecker;
+import org.apache.calcite.rel.hint.HintPredicate;
+import org.apache.calcite.rel.hint.HintPredicates;
+import org.apache.calcite.rel.hint.RelHint;
+import
org.apache.ignite.internal.processors.query.calcite.rel.logical.IgniteLogicalTableScan;
+
+/**
+ * Holds supported SQL hints and their settings.
+ */
+public enum HintDefinition {
+ /** Sets the query engine like H2 or Calcite. */
+ QUERY_ENGINE,
+
+ /** Disables planner rules. */
+ DISABLE_RULE,
+
+ /** Forces expanding of distinct aggregates to join. */
+ EXPAND_DISTINCT_AGG {
+ /** {@inheritDoc} */
+ @Override public HintPredicate predicate() {
+ return HintPredicates.AGGREGATE;
+ }
+
+ /** {@inheritDoc} */
+ @Override public HintOptionChecker optionsChecker() {
+ return HintsConfig.OPTS_CHECK_EMPTY;
+ }
+ },
+
+ /** Disables indexes. */
+ NO_INDEX {
+ /** {@inheritDoc} */
+ @Override public HintPredicate predicate() {
+ return new HintPredicate() {
+ @Override public boolean apply(RelHint hint, RelNode rel) {
+ return rel instanceof IgniteLogicalTableScan;
+ }
+ };
Review Comment:
` return (hint, rel) -> rel instanceof IgniteLogicalTableScan;`
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PlanningContext.java:
##########
@@ -210,6 +232,75 @@ public RexBuilder rexBuilder() {
return unwrap(BaseQueryContext.class).rexBuilder();
}
+ /**
+ * Stores skipped hint and the reason. Also, logs the issue.
+ */
+ public void skippedHint(RelNode relNode, RelHint hint, String reason) {
+ skippedHints.compute(new IgniteBiTuple<>(hint, relNode), (k, sh) -> {
Review Comment:
I don't like the idea of storing problematic hints in planning context.
Planner can create copies of relNode (for example, when we expand or to union
with two table scans) with the same hints and all these duplicated hints will
be stored here. Currentrly, we use skippedHint method only once, to check that
we don't have conflicting NO_INDEX/FORCE_INDEX hints. Can we instead do this
check on validation before planning? In this case patch will be simplifeed and
we can revert changes to `PlanningContext`, `Commons` and some other related
classes.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/IgniteAggregate.java:
##########
@@ -18,7 +18,6 @@
package org.apache.ignite.internal.processors.query.calcite.rel;
import java.util.List;
-
Review Comment:
Redundant change
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/hint/HintDefinition.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.hint;
+
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.hint.HintOptionChecker;
+import org.apache.calcite.rel.hint.HintPredicate;
+import org.apache.calcite.rel.hint.HintPredicates;
+import org.apache.calcite.rel.hint.RelHint;
+import
org.apache.ignite.internal.processors.query.calcite.rel.logical.IgniteLogicalTableScan;
+
+/**
+ * Holds supported SQL hints and their settings.
+ */
+public enum HintDefinition {
+ /** Sets the query engine like H2 or Calcite. */
+ QUERY_ENGINE,
Review Comment:
Let's return comment about regexp (from CalciteQueryProcessor)
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessor.java:
##########
@@ -127,6 +128,9 @@ public class CalciteQueryProcessor extends
GridProcessorAdapter implements Query
defaults = "" + DFLT_IGNITE_CALCITE_PLANNER_TIMEOUT)
public static final String IGNITE_CALCITE_PLANNER_TIMEOUT =
"IGNITE_CALCITE_PLANNER_TIMEOUT";
+ /** */
+ private static final AtomicReference<IgniteLogger> HINTS_LOG_SUPPLIER =
new AtomicReference<>();
Review Comment:
It's a static field and will be filled with first configured logger in JVM
and this is not a good solution. For example tests cannot inject listening
loggers (not using reflection). Using reflection to insert listening logger is
a dirty hack.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
