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 dbfee6eae9 [CALCITE-7792] Failed namespace validation breaks
SqlAdvisorValidator hints and can overflow the stack in getRowType()
dbfee6eae9 is described below
commit dbfee6eae9949443285e7e7e1fb616b4a307d030
Author: Sean Broeder <[email protected]>
AuthorDate: Sat Nov 7 18:18:42 2026 -0800
[CALCITE-7792] Failed namespace validation breaks SqlAdvisorValidator hints
and can overflow the stack in getRowType()
AbstractNamespace.validate()'s finally block unconditionally set status
to VALID even when validateImpl() threw, leaving rowType/type
permanently null. A later getRowType()/getType() call on that namespace
would then recurse into validateNamespace() forever, since it always
finds rowType still null.
Fixed by storing the exception from a failed validateImpl() and
marking the namespace VALID on failure, so a later
getRowType() re-throws the real error instead of unsafely re-entering
validateImpl(), which may have already mutated shared validator state,
or recursing forever into a null rowType.
SqlAdvisorValidator's activeNamespaces guard treated any repeat call to
validateNamespace() as a cycle, even when the repeat call was a harmless
no-op on an already-validated namespace. AbstractNamespace already
handles that safely via its own status field, so the guard is redundant.
The guard also gave wrong results: it set an empty struct type on the
repeat call, so a query such as select ename from (select ?, ename from
emp) returned no completion hints. Removed it and use the namespace's
own cycle detection instead. That detection throws AssertionError, which
the advisor does not mask.
validateNamespace() now masks CalciteException and, on failure, falls
back to an empty struct type so callers can enumerate zero columns.
Also override inferUnknownTypes() and checkRollUp() to swallow
CalciteException from unresolvable sub-expressions, like an unresolvable
column in an incomplete SQL fragment, so a single failing item in a
select list no longer aborts validation of the whole enclosing
namespace. SqlValidatorImpl.checkRollUp() becomes protected so that it
can be overridden.
---
.../calcite/sql/advise/SqlAdvisorValidator.java | 51 +++++++++--
.../calcite/sql/validate/AbstractNamespace.java | 26 +++++-
.../calcite/sql/validate/SqlValidatorImpl.java | 2 +-
.../apache/calcite/sql/test/SqlAdvisorTest.java | 13 +++
.../sql/validate/AbstractNamespaceTest.java | 100 +++++++++++++++++++++
.../org/apache/calcite/test/SqlValidatorTest.java | 34 +++++++
6 files changed, 215 insertions(+), 11 deletions(-)
diff --git
a/core/src/main/java/org/apache/calcite/sql/advise/SqlAdvisorValidator.java
b/core/src/main/java/org/apache/calcite/sql/advise/SqlAdvisorValidator.java
index 4a13b173e8..ce7026020a 100644
--- a/core/src/main/java/org/apache/calcite/sql/advise/SqlAdvisorValidator.java
+++ b/core/src/main/java/org/apache/calcite/sql/advise/SqlAdvisorValidator.java
@@ -35,9 +35,9 @@
import org.apache.calcite.sql.validate.SqlValidatorScope;
import org.apache.calcite.util.Util;
-import java.util.HashSet;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
import java.util.Map;
-import java.util.Set;
/**
* <code>SqlAdvisorValidator</code> is used by {@link SqlAdvisor} to traverse
@@ -48,8 +48,6 @@
public class SqlAdvisorValidator extends SqlValidatorImpl {
//~ Instance fields --------------------------------------------------------
- private final Set<SqlValidatorNamespace> activeNamespaces = new HashSet<>();
-
private final RelDataType emptyStructType =
SqlTypeUtil.createEmptyStructType(typeFactory);
@@ -184,17 +182,54 @@ private void registerId(SqlIdentifier id,
SqlValidatorScope scope) {
}
}
+ /**
+ * Masks type-inference {@link CalciteException} during validation of
+ * sub-expressions so that the advisor can continue providing hints even
+ * when a namespace fails to validate, for example an unknown column in
+ * an incomplete SQL fragment.
+ */
+ @Override protected void inferUnknownTypes(
+ RelDataType inferredType,
+ SqlValidatorScope scope,
+ SqlNode node) {
+ try {
+ super.inferUnknownTypes(inferredType, scope, node);
+ } catch (CalciteException e) {
+ Util.swallow(e, TRACER);
+ }
+ }
+
@Override protected void validateNamespace(final SqlValidatorNamespace
namespace,
RelDataType targetRowType) {
- // Only attempt to validate each namespace once. Otherwise, if
- // validation fails, we may end up cycling.
- if (activeNamespaces.add(namespace)) {
+ // Namespace.validate() already guards against cycles via its own
+ // status field, throwing on re-entry while IN_PROGRESS, and is a
+ // no-op if already VALID.
+ try {
super.validateNamespace(namespace, targetRowType);
- } else {
+ } catch (CalciteException e) {
+ // Mask validation errors so the advisor can continue providing hints
+ // even when a namespace fails to validate. Fall back to an empty struct
+ // type so callers can enumerate zero columns gracefully.
+ Util.swallow(e, TRACER);
namespace.setType(emptyStructType);
}
}
+ /**
+ * Masks {@link CalciteException} during roll-up validation, which is
+ * irrelevant for completion hints and can fail with an unresolvable column,
+ * which would otherwise abort validation of the entire enclosing namespace.
+ */
+ @Override protected void checkRollUp(@Nullable SqlNode grandParent,
+ @Nullable SqlNode parent, @Nullable SqlNode current, SqlValidatorScope
scope,
+ @Nullable String contextClause) {
+ try {
+ super.checkRollUp(grandParent, parent, current, scope, contextClause);
+ } catch (CalciteException e) {
+ Util.swallow(e, TRACER);
+ }
+ }
+
@Override public boolean validateModality(SqlSelect select,
SqlModality modality, boolean fail) {
return true;
diff --git
a/core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java
b/core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java
index 707c460f80..01f105e6fc 100644
--- a/core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java
+++ b/core/src/main/java/org/apache/calcite/sql/validate/AbstractNamespace.java
@@ -48,6 +48,18 @@ abstract class AbstractNamespace implements
SqlValidatorNamespace {
private SqlValidatorImpl.Status status =
SqlValidatorImpl.Status.UNVALIDATED;
+ /**
+ * If {@link #validate} threw, the stored exception is re-thrown by
+ * {@link #getRowType()} so callers surface the real error instead of
+ * recursing forever into a null {@link #rowType}.
+ *
+ * <p>Status is set to {@link SqlValidatorImpl.Status#VALID} on failure,
+ * not {@code UNVALIDATED}. This prevents unsafe re-entry into
+ * {@link #validateImpl(RelDataType)} which may have already partially
mutated
+ * shared validator state.
+ */
+ private @Nullable RuntimeException validationFailure;
+
/**
* Type of the output row, which comprises the name and type of each output
* column. Set on validate.
@@ -89,15 +101,22 @@ abstract class AbstractNamespace implements
SqlValidatorNamespace {
@Override public final void validate(RelDataType targetRowType) {
switch (status) {
case UNVALIDATED:
+ status = SqlValidatorImpl.Status.IN_PROGRESS;
try {
- status = SqlValidatorImpl.Status.IN_PROGRESS;
checkArgument(rowType == null,
"Namespace.rowType must be null before validate has been called");
RelDataType type = validateImpl(targetRowType);
requireNonNull(type, "validateImpl() returned null");
setType(type);
- } finally {
status = SqlValidatorImpl.Status.VALID;
+ } catch (RuntimeException e) {
+ // Mark VALID so later calls do not re-enter validateImpl(),
+ // which may have already partially mutated shared validator state.
+ // Store the exception so getRowType() re-throws the real error
+ // instead of recursing into a null rowType.
+ status = SqlValidatorImpl.Status.VALID;
+ validationFailure = e;
+ throw e;
}
break;
case IN_PROGRESS:
@@ -122,6 +141,9 @@ abstract class AbstractNamespace implements
SqlValidatorNamespace {
@Override public RelDataType getRowType() {
if (rowType == null) {
+ if (validationFailure != null) {
+ throw validationFailure;
+ }
validator.validateNamespace(this, validator.unknownType);
requireNonNull(rowType, "validate must set rowType");
}
diff --git
a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
index 63052a8549..b514acc61d 100644
--- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
+++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
@@ -4948,7 +4948,7 @@ private static SqlNode stripDot(SqlNode node) {
return res;
}
- private void checkRollUp(@Nullable SqlNode grandParent, @Nullable SqlNode
parent,
+ protected void checkRollUp(@Nullable SqlNode grandParent, @Nullable SqlNode
parent,
@Nullable SqlNode current, SqlValidatorScope scope, @Nullable String
contextClause) {
current = stripAs(current);
if (current instanceof SqlCall
diff --git a/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
b/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
index 3b086d50ff..45a4ebd1a2 100644
--- a/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
+++ b/core/src/test/java/org/apache/calcite/sql/test/SqlAdvisorTest.java
@@ -1317,6 +1317,19 @@ private void checkSimpleParserQuotedIdImpl(Fixture
fixture) {
ImmutableMap.of("COLUMN(count)", "`count`"));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7792";>[CALCITE-7792]
+ * Failed namespace validation breaks SqlAdvisorValidator hints and can
+ * overflow the stack in getRowType()</a>.
+ *
+ * <p>The sub-query fails to validate, because the type of {@code ?} cannot
+ * be inferred, but the advisor still offers the columns it can resolve. */
+ @Test void testAdviceSubQueryWithDynamicParam() {
+ final String sql = "select ename^ from (select ?, ename from emp)";
+ fixture().withSql(sql)
+ .assertComplete("COLUMN(ENAME)\n", "ename");
+ }
+
@Test void testAdviceEmptyFrom() {
String sql = "select * from^";
fixture().withSql(sql).withLex(Lex.JAVA)
diff --git
a/core/src/test/java/org/apache/calcite/sql/validate/AbstractNamespaceTest.java
b/core/src/test/java/org/apache/calcite/sql/validate/AbstractNamespaceTest.java
new file mode 100644
index 0000000000..5ed9c725e6
--- /dev/null
+++
b/core/src/test/java/org/apache/calcite/sql/validate/AbstractNamespaceTest.java
@@ -0,0 +1,100 @@
+/*
+ * 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.sql.validate;
+
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.test.SqlTestFactory;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.sameInstance;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/** Unit tests for {@link AbstractNamespace}. */
+class AbstractNamespaceTest {
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7792";>[CALCITE-7792]
+ * Failed namespace validation breaks SqlAdvisorValidator hints and can
+ * overflow the stack in getRowType()</a>.
+ *
+ * <p>After {@code validateImpl()} fails, both {@code getRowType()} and
+ * {@code getType()} re-throw the original exception. */
+ @Test void testFailureIsRethrownByGetRowTypeAndGetType() {
+ final FailingNamespace namespace = newNamespace();
+ final RuntimeException failure =
+ assertThrows(RuntimeException.class,
+ () -> namespace.validate(unknownType(namespace)));
+ assertThat(failure, sameInstance(namespace.failure));
+
+ assertThat(assertThrows(RuntimeException.class, namespace::getRowType),
+ sameInstance(namespace.failure));
+ assertThat(assertThrows(RuntimeException.class, namespace::getType),
+ sameInstance(namespace.failure));
+ }
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7792";>[CALCITE-7792]
+ * Failed namespace validation breaks SqlAdvisorValidator hints and can
+ * overflow the stack in getRowType()</a>.
+ *
+ * <p>A namespace whose validation failed is marked VALID, so it is not
+ * validated a second time; {@code validateImpl()} may already have changed
+ * the validator's state. */
+ @Test void testFailedNamespaceIsNotValidatedAgain() {
+ final FailingNamespace namespace = newNamespace();
+ assertThrows(RuntimeException.class,
+ () -> namespace.validate(unknownType(namespace)));
+ assertThat(namespace.validateImplCount, is(1));
+
+ // Validating again is a no-op; the error comes from the row type.
+ namespace.validate(unknownType(namespace));
+ assertThrows(RuntimeException.class, namespace::getRowType);
+ assertThat(namespace.validateImplCount, is(1));
+ }
+
+ private static FailingNamespace newNamespace() {
+ return new FailingNamespace(
+ (SqlValidatorImpl) SqlTestFactory.INSTANCE.createValidator());
+ }
+
+ private static RelDataType unknownType(FailingNamespace namespace) {
+ return ((SqlValidatorImpl) namespace.getValidator()).unknownType;
+ }
+
+ /** Namespace whose validation always fails. */
+ private static class FailingNamespace extends AbstractNamespace {
+ final RuntimeException failure = new IllegalStateException("cannot
validate");
+ int validateImplCount;
+
+ FailingNamespace(SqlValidatorImpl validator) {
+ super(validator, null);
+ }
+
+ @Override protected RelDataType validateImpl(RelDataType targetRowType) {
+ ++validateImplCount;
+ throw failure;
+ }
+
+ @Override public @Nullable SqlNode getNode() {
+ return null;
+ }
+ }
+}
diff --git a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
index 1453459c22..2cad0239cc 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
@@ -65,6 +65,7 @@
import org.apache.calcite.sql.validate.SqlValidator;
import org.apache.calcite.sql.validate.SqlValidatorCatalogReader;
import org.apache.calcite.sql.validate.SqlValidatorImpl;
+import org.apache.calcite.sql.validate.SqlValidatorNamespace;
import org.apache.calcite.sql.validate.SqlValidatorScope;
import org.apache.calcite.sql.validate.SqlValidatorUtil;
import org.apache.calcite.test.catalog.CountingFactory;
@@ -16437,4 +16438,37 @@ private static class RownumRewritingValidator extends
SqlValidatorImpl {
sql("select * from unnest(array[(1, 'a'), (2, 'b')], array[10, 20]) as
t(p, q, r)")
.type("RecordType(INTEGER P, CHAR(1) Q, INTEGER R) NOT NULL");
}
+
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-7792";>[CALCITE-7792]
+ * Failed namespace validation breaks SqlAdvisorValidator hints and can
+ * overflow the stack in getRowType()</a>.
+ *
+ * <p>After validateImpl() fails, a later getRowType() re-throws the stored
+ * error instead of recursing. */
+ @Test void testNamespaceRethrowsStoredFailure() throws Exception {
+ final String sql = "select * from (select bad_col from emp) as t";
+ final SqlNode topNode = fixture().factory.createParser(sql).parseQuery();
+
+ // Grab the inner SELECT's SqlNode before validation, so we can look up its
+ // namespace after the expected failure.
+ final SqlSelect outerSelect = (SqlSelect) topNode;
+ final SqlCall asCall = (SqlCall) outerSelect.getFrom();
+ final SqlNode innerSelect = asCall.operand(0);
+
+ final SqlValidator validator = fixture().factory.createValidator();
+
+ // First validation attempt fails with a clear error about the bad column.
+ final Exception firstFailure =
+ assertThrows(Exception.class, () -> validator.validate(topNode));
+ assertThat(firstFailure.getMessage(), containsString("BAD_COL"));
+
+ final SqlValidatorNamespace namespace =
validator.getNamespace(innerSelect);
+ assertThat(namespace, notNullValue());
+
+ // getRowType() call re-throws the stored error rather than recursing into
a null rowType.
+ final Exception secondFailure =
+ assertThrows(Exception.class, namespace::getRowType);
+ assertThat(secondFailure.getMessage(), containsString("BAD_COL"));
+ }
}
