Fix Juian's review comments address

core/src/main/codegen/templates/Parser.jj

@@ -1122,10 +1122,9 @@ SqlNode TableRefWithHintsOpt() :
     tableRef = CompoundIdentifier()
     [
         LOOKAHEAD(2)
-        <WITH> <LPAREN>
-        { s.add(getPos()); }
+        <HINT_BEG>
         CommaSepatatedSqlHints(hints)
-        <RPAREN>
+        <COMMENT_END>
         {
             hintList = new SqlNodeList(hints, s.addAll(hints).end(this));
             tableRef = new SqlTableRef(Span.of(tableRef, hintList).pos(),

core/src/main/java/org/apache/calcite/plan/RelOptRuleCall.java

@@ -219,6 +219,10 @@ public List<RelNode> getParents() {
    * rel.getTraits()</code> will be copied from <code>
    * this.rels[0].getTraitSet()</code>.
    *
+   * <p>The implementation of this method also guarantees that the original
+   * relational expression (that is, this.rels[0]) has its hints propagated to
+   * the new relational expression (rel).
+   *
    * @param rel   Relational expression equivalent to the root relational
    *              expression of the rule call, {@code call.rels(0)}
    * @param equiv Map of other equivalences

core/src/main/java/org/apache/calcite/plan/RelOptUtil.java

@@ -44,6 +44,7 @@
 import org.apache.calcite.rel.externalize.RelJsonWriter;
 import org.apache.calcite.rel.externalize.RelWriterImpl;
 import org.apache.calcite.rel.externalize.RelXmlWriter;
+import org.apache.calcite.rel.hint.RelWithHint;
 import org.apache.calcite.rel.logical.LogicalAggregate;
 import org.apache.calcite.rel.logical.LogicalCalc;
 import org.apache.calcite.rel.logical.LogicalFilter;
@@ -383,6 +384,25 @@ public static void verifyTypeEquivalence(
     throw new AssertionError(s);
   }
 
+  /**
+   * Copy the {@link org.apache.calcite.rel.hint.RelHint}s from {@code originalRel}
+   * to {@code newRel} if both of them are {@link RelWithHint}.
+   *
+   * @param originalRel the original relational expression
+   * @param newRel      the new relational expression
+   * @return A copy of {@code newRel} with hints of {@code originalRel}, or {@code newRel}
+   * directly if one of them are not {@link RelWithHint}.
+   */
+  public static RelNode copyRelHints(RelNode originalRel, RelNode newRel) {
+    if (originalRel instanceof RelWithHint
+        && newRel instanceof RelWithHint
+        && ((RelWithHint) originalRel).getHints().size() > 0) {
+      return ((RelWithHint) newRel)
+          .attachHints(((RelWithHint) originalRel).getHints());
+    }
+    return newRel;
+  }
+
   /**
    * Returns a permutation describing where output fields come from. In
    * the returned map, value of {@code map.getTargetOpt(i)} is {@code n} if

core/src/main/java/org/apache/calcite/plan/hep/HepRuleCall.java

@@ -55,8 +55,9 @@ public class HepRuleCall extends RelOptRuleCall {
   public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv) {
     final RelNode rel0 = rels[0];
     RelOptUtil.verifyTypeEquivalence(rel0, rel, rel0);
+    rel = RelOptUtil.copyRelHints(rel0, rel);
     results.add(rel);
-    rel(0).getCluster().invalidateMetadataQuery();
+    rel0.getCluster().invalidateMetadataQuery();
   }
 
   List<RelNode> getResults() {

core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java

@@ -20,6 +20,7 @@
 import org.apache.calcite.plan.RelOptRuleCall;
 import org.apache.calcite.plan.RelOptRuleOperand;
 import org.apache.calcite.plan.RelOptRuleOperandChildPolicy;
+import org.apache.calcite.plan.RelOptUtil;
 import org.apache.calcite.rel.RelNode;
 
 import com.google.common.collect.ImmutableList;
@@ -88,6 +89,7 @@ protected VolcanoRuleCall(
 
   // implement RelOptRuleCall
   public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv) {
+    rel = RelOptUtil.copyRelHints(rels[0], rel);
     if (LOGGER.isDebugEnabled()) {
       LOGGER.debug("Transform to: rel#{} via {}{}", rel.getId(), getRule(),
           equiv.isEmpty() ? "" : " with equivalences " + equiv);

core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java

@@ -69,7 +69,6 @@
 import org.apache.calcite.rel.core.Project;
 import org.apache.calcite.rel.core.Sort;
 import org.apache.calcite.rel.core.TableScan;
-import org.apache.calcite.rel.hint.HintStrategies;
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeFactory;
 import org.apache.calcite.rel.type.RelDataTypeField;
@@ -1019,7 +1018,7 @@ private PreparedResult prepare_(Supplier<RelNode> fn,
         SqlToRelConverter.Config config) {
       final RelOptCluster cluster = prepare.createCluster(planner, rexBuilder);
       return new SqlToRelConverter(this, validator, catalogReader, cluster,
-          convertletTable, HintStrategies.EMPTY, config);
+          convertletTable, config);
     }
 
     @Override public RelNode flattenTypes(

core/src/main/java/org/apache/calcite/prepare/PlannerImpl.java

@@ -35,7 +35,6 @@
 import org.apache.calcite.rel.RelCollationTraitDef;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.RelRoot;
-import org.apache.calcite.rel.hint.HintStrategies;
 import org.apache.calcite.rel.metadata.CachingRelMetadataProvider;
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeSystem;
@@ -250,7 +249,7 @@ public RelRoot rel(SqlNode sql) throws RelConversionException {
         .build();
     final SqlToRelConverter sqlToRelConverter =
         new SqlToRelConverter(this, validator,
-            createCatalogReader(), cluster, convertletTable, HintStrategies.EMPTY, config);
+            createCatalogReader(), cluster, convertletTable, config);
     root =
         sqlToRelConverter.convertQuery(validatedSqlNode, false, true);
     root = root.withRel(sqlToRelConverter.flattenTypes(root.rel, true));
@@ -307,7 +306,7 @@ public RelRoot expandView(RelDataType rowType, String queryString,
         .build();
     final SqlToRelConverter sqlToRelConverter =
         new SqlToRelConverter(this, validator,
-            catalogReader, cluster, convertletTable, HintStrategies.EMPTY, config);
+            catalogReader, cluster, convertletTable, config);
 
     final RelRoot root =
         sqlToRelConverter.convertQuery(sqlNode, true, false);

core/src/main/java/org/apache/calcite/rel/RelRoot.java

@@ -16,6 +16,7 @@
  */
 package org.apache.calcite.rel;
 
+import org.apache.calcite.plan.RelOptUtil;
 import org.apache.calcite.rel.hint.RelHint;
 import org.apache.calcite.rel.logical.LogicalProject;
 import org.apache.calcite.rel.type.RelDataType;
@@ -166,7 +167,7 @@ public RelNode project(boolean force) {
     for (Pair<Integer, String> field : fields) {
       projects.add(rexBuilder.makeInputRef(rel, field.left));
     }
-    return LogicalProject.create(rel, projects, Pair.right(fields));
+    return RelOptUtil.copyRelHints(rel, LogicalProject.create(rel, projects, Pair.right(fields)));
   }
 
   public boolean isNameTrivial() {

core/src/main/java/org/apache/calcite/rel/hint/ExplicitHintMatcher.java

@@ -0,0 +1,41 @@
+/*
+ * 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.calcite.rel.hint;
+
+import org.apache.calcite.linq4j.function.Function2;
+import org.apache.calcite.rel.RelNode;
+
+/**
+ * A function to customize whether a relational expression should match a hint.
+ *
+ * <p>Usually you may not need to implement this function, a {@link NodeTypeHintStrategy} is enough
+ * for most of the {@link RelHint}s.
+ *
+ * <p>Some of the hints can only be matched to the relational
+ * expression with special match conditions(not only the relational expression type).
+ * i.e. "hash_join(r, st)", this hint can only be applied to JOIN expression that
+ * has "r" and "st" as the input table names. To implement this, you may need to customize an
+ * {@link ExplicitHintStrategy} with an {@link ExplicitHintMatcher}.
+ *
+ * @see ExplicitHintStrategy
+ * @see HintStrategyTable
+ */
+public interface ExplicitHintMatcher extends Function2<RelHint, RelNode, Boolean> {
+}
+
+// End ExplicitHintMatcher.java

core/src/main/java/org/apache/calcite/rel/hint/ExplicitHintStrategy.java

@@ -0,0 +1,49 @@
+/*
+ * 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.calcite.rel.hint;
+
+import org.apache.calcite.rel.RelNode;
+
+/**
+ * A hint strategy whose rules are totally customized.
+ *
+ * @see ExplicitHintMatcher
+ */
+public class ExplicitHintStrategy implements HintStrategy {
+  //~ Instance fields --------------------------------------------------------
+
+  private final ExplicitHintMatcher matcher;
+
+  /**
+   * Creates a ExplicitHintStrategy with specified {@code matcher}.
+   *
+   * @param matcher ExplicitHintMatcher instance to test
+   *                if a hint can be applied to a rel.
+   */
+  ExplicitHintStrategy(ExplicitHintMatcher matcher) {
+    this.matcher = matcher;
+  }
+
+  //~ Methods ----------------------------------------------------------------
+
+  @Override public boolean supportsRel(RelHint hint, RelNode rel) {
+    return this.matcher.apply(hint, rel);
+  }
+}
+
+// End ExplicitHintStrategy.java

core/src/main/java/org/apache/calcite/rel/hint/HintStrategies.java

@@ -14,123 +14,48 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.apache.calcite.rel.hint;
-
-import com.google.common.collect.ImmutableMap;
 
-import java.util.HashMap;
-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
-import java.util.Objects;
-import java.util.stream.Collectors;
+package org.apache.calcite.rel.hint;
 
 /**
- * {@code HintStrategy} collection indicating which kind of
- * {@link org.apache.calcite.rel.RelNode} the hint can apply to.
- *
- * <p>Typically, every supported hints should register a {@code HintStrategy}
- * in this collection. For example, {@link HintStrategy#JOIN} implies that this hint
- * would be propagated and applied to the {@link org.apache.calcite.rel.core.Join}
- * relational expressions.
- *
- * <p>Only one kind of {@code HintStrategy} is supported for each registered hint
- * right now. The matching for hint name is case in-sensitive.
- *
- * @see HintStrategy
+ * A collection of hint strategies.
  */
-public class HintStrategies {
-  /** Empty strategies. */
-  // Need to replace the EMPTY with DEFAULT if we have any hint implementations.
-  public static final HintStrategies EMPTY = new HintStrategies(new HashMap<>());
-
-  /** Mapping from hint name to strategy. */
-  private final Map<Key, HintStrategy> hintStrategyMap;
-
-  private HintStrategies(Map<Key, HintStrategy> strategies) {
-    this.hintStrategyMap = ImmutableMap.copyOf(strategies);
-  }
+public abstract class HintStrategies {
+  /** A hint strategy that indicates a hint can only be used to
+   * the whole query(no specific nodes). */
+  public static final HintStrategy QUERY =
+      new NodeTypeHintStrategy(NodeTypeHintStrategy.NodeType.QUERY);
+
+  /** A hint strategy that indicates a hint can only be used to
+   * {@link org.apache.calcite.rel.core.Join} nodes. */
+  public static final HintStrategy JOIN =
+      new NodeTypeHintStrategy(NodeTypeHintStrategy.NodeType.JOIN);
+
+  /** A hint strategy that indicates a hint can only be used to
+   * {@link org.apache.calcite.rel.core.TableScan} nodes. */
+  public static final HintStrategy TABLE_SCAN =
+      new NodeTypeHintStrategy(NodeTypeHintStrategy.NodeType.TABLE_SCAN);
+
+  /** A hint strategy that indicates a hint can only be used to
+   * {@link org.apache.calcite.rel.core.Project} nodes. */
+  public static final HintStrategy PROJECT =
+      new NodeTypeHintStrategy(NodeTypeHintStrategy.NodeType.PROJECT);
 
   /**
-   * Apply this {@link HintStrategies} to the given relational
-   * expression for the {@code hints}.
-   *
-   * @param hints the hints that may attach to the {@code rel}.
-   * @param clazz the relational expression type class.
-   * @return A hints list that can be attached to the {@code rel}.
+   * Create a hint strategy from a specific matcher whose rules are totally customized.
+   * @param matcher The strategy matcher
+   * @return A ExplicitHintStrategy instance.
    */
-  public List<RelHint> apply(List<RelHint> hints, Class clazz) {
-    return hints.stream()
-        .filter(relHint -> supportsRel(relHint.hintName, clazz))
-        .collect(Collectors.toList());
-  }
-
-  /**
-   * Check if the give hint name is valid.
-   * @param name the hint name.
-   */
-  public void validateHint(String name) {
-    if (!this.hintStrategyMap.containsKey(Key.of(name))) {
-      throw new RuntimeException("Hint: " + name + " should be registered in the HintStrategies.");
-    }
-  }
-
-  private boolean supportsRel(String hintName, Class relClazz) {
-    final Key key = Key.of(hintName);
-    assert this.hintStrategyMap.containsKey(key);
-    return this.hintStrategyMap.get(Key.of(hintName)).supportsRel(relClazz);
+  public static HintStrategy explicit(ExplicitHintMatcher matcher) {
+    return new ExplicitHintStrategy(matcher);
   }
 
   /**
-   * @return A strategies builder.
+   * Creates a HintStrategyCascade instance whose strategy rules are satisfied only if
+   * all the {@code hintStrategies} are satisfied.
    */
-  public static Builder builder() {
-    return new Builder();
-  }
-
-  /**
-   * Key used to keep the strategies which ignores the case sensitivity.
-   */
-  private static class Key {
-    private String name;
-    private Key(String name) {
-      this.name = name;
-    }
-
-    static Key of(String name) {
-      return new Key(name.toLowerCase(Locale.ROOT));
-    }
-
-    @Override public boolean equals(Object obj) {
-      if (!(obj instanceof Key)) {
-        return false;
-      }
-      return Objects.equals(this.name, ((Key) obj).name);
-    }
-
-    @Override public int hashCode() {
-      return this.name.hashCode();
-    }
-  }
-
-  /**
-   * Builder for {@code HintStrategies}.
-   */
-  public static class Builder {
-    private Map<Key, HintStrategy> hintStrategyMap;
-
-    public Builder() {
-      this.hintStrategyMap = new HashMap<>();
-    }
-
-    public Builder addHintStrategy(String hintName, HintStrategy strategy) {
-      this.hintStrategyMap.put(Key.of(hintName), strategy);
-      return this;
-    }
-
-    public HintStrategies build() {
-      return new HintStrategies(this.hintStrategyMap);
-    }
+  public static HintStrategy cascade(HintStrategy... hintStrategies) {
+    return new HintStrategyCascade(hintStrategies);
   }
 }