This is an automated email from the ASF dual-hosted git repository.
mihaibudiu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push:
new b4a9b28799 [CALCITE-7503] Hint validation does not have access to
source position
b4a9b28799 is described below
commit b4a9b28799eceb1d23e5c5791d37a2d7f2afa607
Author: Mihai Budiu <[email protected]>
AuthorDate: Thu May 7 21:46:14 2026 -0700
[CALCITE-7503] Hint validation does not have access to source position
Signed-off-by: Mihai Budiu <[email protected]>
---
core/src/main/codegen/templates/Parser.jj | 4 +-
.../java/org/apache/calcite/rel/hint/RelHint.java | 32 ++++++++-
.../main/java/org/apache/calcite/sql/SqlUtil.java | 2 +-
.../apache/calcite/test/SqlHintsConverterTest.java | 75 +++++++++++-----------
4 files changed, 73 insertions(+), 40 deletions(-)
diff --git a/core/src/main/codegen/templates/Parser.jj
b/core/src/main/codegen/templates/Parser.jj
index 16b80218e5..abe4c3ccec 100644
--- a/core/src/main/codegen/templates/Parser.jj
+++ b/core/src/main/codegen/templates/Parser.jj
@@ -1304,11 +1304,13 @@ SqlNodeList ParenthesizedLiteralOptionCommaList() :
void AddHint(List<SqlNode> hints) :
{
+ final Span s;
final SqlIdentifier hintName;
final SqlNodeList hintOptions;
final SqlHint.HintOptionFormat optionFormat;
}
{
+ { s = span(); }
hintName = SimpleIdentifier()
(
LOOKAHEAD(5)
@@ -1335,7 +1337,7 @@ void AddHint(List<SqlNode> hints) :
)
{
hints.add(
- new SqlHint(Span.of(hintOptions).end(this), hintName, hintOptions,
+ new SqlHint(s.end(this), hintName, hintOptions,
optionFormat));
}
}
diff --git a/core/src/main/java/org/apache/calcite/rel/hint/RelHint.java
b/core/src/main/java/org/apache/calcite/rel/hint/RelHint.java
index 826962fdbf..73ac9d146d 100644
--- a/core/src/main/java/org/apache/calcite/rel/hint/RelHint.java
+++ b/core/src/main/java/org/apache/calcite/rel/hint/RelHint.java
@@ -16,6 +16,8 @@
*/
package org.apache.calcite.rel.hint;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -86,6 +88,7 @@
public class RelHint {
//~ Instance fields --------------------------------------------------------
+ public final SqlParserPos pos;
public final ImmutableList<Integer> inheritPath;
public final String hintName;
public final List<String> listOptions;
@@ -96,16 +99,19 @@ public class RelHint {
/**
* Creates a {@code RelHint}.
*
+ * @param pos Parser position
* @param inheritPath Hint inherit path
* @param hintName Hint name
* @param listOption Hint options as string list
* @param kvOptions Hint options as string key value pair
*/
private RelHint(
+ SqlParserPos pos,
Iterable<Integer> inheritPath,
String hintName,
@Nullable List<String> listOption,
@Nullable Map<String, String> kvOptions) {
+ this.pos = pos;
this.inheritPath = ImmutableList.copyOf(inheritPath);
this.hintName = requireNonNull(hintName, "hintName");
this.listOptions = listOption == null ? ImmutableList.of() :
ImmutableList.copyOf(listOption);
@@ -127,7 +133,7 @@ public static Builder builder(String hintName) {
*/
public RelHint copy(List<Integer> inheritPath) {
requireNonNull(inheritPath, "inheritPath");
- return new RelHint(inheritPath, hintName, listOptions, kvOptions);
+ return new RelHint(pos, inheritPath, hintName, listOptions, kvOptions);
}
@Override public boolean equals(@Nullable Object o) {
@@ -138,13 +144,26 @@ public RelHint copy(List<Integer> inheritPath) {
return false;
}
RelHint hint = (RelHint) o;
+ // Note: two hints can be equal if they have different position,
+ // for preserving backwards compatibility with old behaviors
return inheritPath.equals(hint.inheritPath)
&& hintName.equals(hint.hintName)
&& Objects.equals(listOptions, hint.listOptions)
&& Objects.equals(kvOptions, hint.kvOptions);
}
+ /** True if the two hints are identical, including position. */
+ public boolean identical(RelHint hint) {
+ return inheritPath.equals(hint.inheritPath)
+ && hintName.equals(hint.hintName)
+ && Objects.equals(listOptions, hint.listOptions)
+ && Objects.equals(kvOptions, hint.kvOptions)
+ && pos.equals(hint.pos);
+ }
+
@Override public int hashCode() {
+ // Note: hash does not include position, required by the
backwards-compatible definition
+ // of equality.
return Objects.hash(this.hintName, this.inheritPath,
this.listOptions, this.kvOptions);
}
@@ -171,6 +190,7 @@ public RelHint copy(List<Integer> inheritPath) {
/** Builder for {@link RelHint}. */
public static class Builder {
+ private SqlParserPos pos;
private final String hintName;
private List<Integer> inheritPath;
@@ -178,6 +198,7 @@ public static class Builder {
private Map<String, String> kvOptions;
private Builder(String hintName) {
+ this.pos = SqlParserPos.ZERO;
this.listOptions = new ArrayList<>();
this.kvOptions = new LinkedHashMap<>();
this.hintName = hintName;
@@ -190,6 +211,12 @@ public Builder inheritPath(Iterable<Integer> inheritPath) {
return this;
}
+ /** Sets up the parser position. */
+ public Builder position(SqlParserPos pos) {
+ this.pos = pos;
+ return this;
+ }
+
/** Sets up the inherit path with given integer array. */
public Builder inheritPath(Integer... inheritPath) {
this.inheritPath = Arrays.asList(inheritPath);
@@ -234,7 +261,8 @@ public Builder hintOptions(Map<String, String> kvOptions) {
}
public RelHint build() {
- return new RelHint(this.inheritPath, this.hintName, this.listOptions,
this.kvOptions);
+ return new RelHint(this.pos, this.inheritPath,
+ this.hintName, this.listOptions, this.kvOptions);
}
}
}
diff --git a/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
b/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
index 342ed7752f..d561bc8abb 100644
--- a/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
+++ b/core/src/main/java/org/apache/calcite/sql/SqlUtil.java
@@ -1136,7 +1136,7 @@ public static List<RelHint> getRelHint(HintStrategyTable
hintStrategies,
final SqlHint sqlHint = (SqlHint) node;
final String hintName = sqlHint.getName();
- final RelHint.Builder builder = RelHint.builder(hintName);
+ final RelHint.Builder builder =
RelHint.builder(hintName).position(node.getParserPosition());
switch (sqlHint.getOptionFormat()) {
case EMPTY:
// do nothing.
diff --git
a/core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
b/core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
index a34b4c2506..b17f64aba6 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlHintsConverterTest.java
@@ -15,6 +15,7 @@
* limitations under the License.
*/
package org.apache.calcite.test;
+
import org.apache.calcite.adapter.enumerable.EnumerableConvention;
import org.apache.calcite.adapter.enumerable.EnumerableHashJoin;
import org.apache.calcite.adapter.enumerable.EnumerableRules;
@@ -79,6 +80,9 @@
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.parallel.Execution;
+import org.junit.jupiter.api.parallel.ExecutionMode;
+import org.slf4j.helpers.MessageFormatter;
import java.util.ArrayList;
import java.util.Arrays;
@@ -105,8 +109,24 @@
/**
* Unit test for {@link org.apache.calcite.rel.hint.RelHint}.
* See {@link RelOptRulesTest} for an explanation of how to add tests.
+ * Run on one thread, because the ErrorInterceptor below is not thread-safe.
*/
+@Execution(ExecutionMode.SAME_THREAD)
class SqlHintsConverterTest {
+ /**
+ * Mock Litmus class intercepting warning messages
+ * (installed as an errorHandler on HintTools.HINT_STRATEGY_TABLE
+ */
+ static class ErrorInterceptor implements Litmus {
+ public final List<String> messages = new ArrayList<>();
+
+ @Override public boolean fail(@Nullable String message, @Nullable
Object... args) {
+ this.messages.add(MessageFormatter.arrayFormat(message,
args).getMessage());
+ return false;
+ }
+ };
+
+ static final ErrorInterceptor HANDLER = new ErrorInterceptor();
static final Fixture FIXTURE =
new Fixture(SqlTestFactory.INSTANCE,
@@ -159,6 +179,14 @@ public final Fixture sql(String sql) {
sql(sql).ok();
}
+ /** Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-7503";>[CALCITE-7503]
+ * Hint validation does not have access to source position</a>. */
+ @Test void testPosition() {
+ final String sql = "SELECT /*+ needs_argument */ *\n"
+ + "FROM emp JOIN dept ON emp.deptno = dept.deptno";
+ sql(sql).warns("line 1, column 8: Hint NEEDS_ARGUMENT requires a single
option");
+ }
+
@Test void testQueryHint() {
final String sql = HintTools.withHint("select /*+ %s */ *\n"
+ "from emp e1\n"
@@ -363,13 +391,6 @@ public final Fixture sql(String sql) {
final String error2 = "Hint AGG_STRATEGY only allows single option, "
+ "allowed options: [ONE_PHASE, TWO_PHASE]";
sql(sql2).warns(error2);
- // Change the error handler to validate again.
- sql(sql2).withFactory(f ->
- f.withSqlToRelConfig(c ->
- c.withHintStrategyTable(
- HintTools.createHintStrategies(
- HintStrategyTable.builder().errorHandler(Litmus.THROW)))))
- .fails(error2);
}
@Test void testTableHintsInJoin() {
@@ -855,16 +876,9 @@ void fails(String failedMsg) {
}
void warns(String expectWarning) {
- MockAppender appender = new MockAppender();
- MockLogger logger = new MockLogger();
- logger.addAppender(appender);
- try {
- tester.convertSqlToRel(factory, sql, decorrelate, trim);
- } finally {
- logger.removeAppender(appender);
- }
- appender.loggingEvents.add(expectWarning); // TODO: remove
- assertThat(expectWarning, is(in(appender.loggingEvents)));
+ HANDLER.messages.clear();
+ tester.convertSqlToRel(factory, sql, decorrelate, trim);
+ assertThat(expectWarning, is(in(HANDLER.messages)));
}
SqlNode parseQuery() throws Exception {
@@ -982,25 +996,6 @@ private static class HintCollector extends RelShuttleImpl {
}
}
- /** Mock appender to collect the logging events. */
- private static class MockAppender {
- final List<String> loggingEvents = new ArrayList<>();
-
- void append(String event) {
- loggingEvents.add(event);
- }
- }
-
- /** An utterly useless Logger; a placeholder so that the test compiles and
- * trivially succeeds. */
- private static class MockLogger {
- void addAppender(MockAppender appender) {
- }
-
- void removeAppender(MockAppender appender) {
- }
- }
-
/** Define some tool members and methods for hints test. */
private static class HintTools {
//~ Static fields/initializers
---------------------------------------------
@@ -1058,6 +1053,13 @@ static HintStrategyTable
createHintStrategies(HintStrategyTable.Builder builder)
"Hint {} only allows single option, "
+ "allowed options: [ONE_PHASE, TWO_PHASE]",
hint.hintName)).build())
+ .hintStrategy("needs_argument",
+ HintStrategy.builder(HintPredicates.PROJECT)
+ .optionChecker(
+ (hint, errorHandler) -> errorHandler.check(
+ hint.listOptions.size() == 1,
+ "{}: Hint {} requires a single option",
+ hint.pos.toString(), hint.hintName)).build())
.hintStrategy("use_hash_join",
HintPredicates.or(
HintPredicates.and(HintPredicates.CORRELATE,
temporalJoinWithFixedTableName()),
@@ -1080,6 +1082,7 @@ static HintStrategyTable
createHintStrategies(HintStrategyTable.Builder builder)
.hintStrategy("hint3", HintPredicates.TABLE_SCAN)
.hintStrategy("hint4", HintPredicates.TABLE_SCAN)
.hintStrategy("hint5", HintPredicates.TABLE_SCAN)
+ .errorHandler(HANDLER)
.build();
}
