[
https://issues.apache.org/jira/browse/CALCITE-7792?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Sean Broeder updated CALCITE-7792:
----------------------------------
Description:
The finally block in AbstractNamespace.validate() sets the status to VALID even
when validateImpl() throws. That leaves rowType null. A later getRowType() on
the same namespace calls validateNamespace(), which does nothing because the
status is already VALID. validateNamespace() then calls getType(), which calls
getRowType() again. The result is a StackOverflowError in place of the original
error.
For example:
{code:java}
select * from (select bad_col from emp) as t
{code}
Validating this throws "Column 'BAD_COL' not found in any table" the first
time, as expected. But a second, independent getRowType() call on the inner
SELECT's namespace then throws StackOverflowError instead of re-surfacing that
error.
Ordinary SQL through a single validate() call does not hit this. It needs a
caller that catches the exception and keeps going, such as a validator
extension that probes a namespace and treats failure as "no match", or code
that asks for a namespace's type after a failed validation. SqlAdvisorValidator
is such a caller, because it validates incomplete SQL and must tolerate
namespaces that fail.
Setting the status back to UNVALIDATED on failure is not enough. validateImpl()
may already have changed shared validator state, so entering it a second time
is unsafe. Instead, store the exception and mark the namespace VALID. A later
getRowType() or getType() re-throws the original error.
In SqlAdvisorValidator:
- Remove the activeNamespaces guard. It treated any repeat call to
validateNamespace() as a cycle, but AbstractNamespace already handles repeat
calls through its own status field.
- validateNamespace() masks CalciteException and falls back to an empty struct
type.
- Override inferUnknownTypes() and checkRollUp() to swallow CalciteException,
so one unresolvable item doesn't abort validation of the whole enclosing
namespace. SqlValidatorImpl.checkRollUp() becomes protected forthis.
The namespace's cycle check throws AssertionError, which the advisor does not
mask.
was:
AbstractNamespace.validate()'s finally block sets status = VALID even when
validateImpl() throws, leaving rowType null. A subsequent getRowType() call on
that same namespace then recurses forever rather than resurfacing the original
error. It sees rowType == null and calls validateNamespace(), which calls
namespace.validate(...) and since status is already VALID, it calls
namespace.getType() -> getRowType() again, ending in a StackOverflowError.
example:
{code:java}
select * from (select bad_col from emp) as t
{code}
Validating this throws "Column 'BAD_COL' not found in any table" the first
time, as expected. But a second, independent getRowType() call on the inner
SELECT's namespace then throws StackOverflowError instead of re-surfacing that
error.
Reachability: this isn't triggered by ordinary correlated SQL through a single
top-level validate() call — I checked a CTE referenced twice via correlated
EXISTS clauses and two independently-correlated subqueries over the same
derived-table. Both surface the clean underlying error, not this bug. Calcite's
own internal re-entrant validate()/getRowType() calls only land on namespaces
that haven't failed yet, and a genuine failure aborts the whole top-level
validate() call before anything gets a second look at that same namespace.
The bug needs something that catches an exception from probing a namespace and
keeps going. In my case, this is a validator extension that speculatively
resolves an identifier against several candidate scopes and treats a resolution
failure as "not a match" rather than a hard error. An external caller that
revalidates the same parsed query or asks for a namespace's type again after an
earlier validation attempt on it would also hit the issue.
This problem can be mitigated if status is set conditionally in the finally
block and I see no impact on standard Calcite users in doing so, so I am hoping
this is agreeable.
{code:java}
status = succeeded ? SqlValidatorImpl.Status.VALID :
SqlValidatorImpl.Status.UNVALIDATED;
{code}
> Failed namespace validation leaves rowType null and causes stack overflow in
> getRowType()
> -----------------------------------------------------------------------------------------
>
> Key: CALCITE-7792
> URL: https://issues.apache.org/jira/browse/CALCITE-7792
> Project: Calcite
> Issue Type: Bug
> Components: core
> Affects Versions: 1.42.0
> Reporter: Sean Broeder
> Assignee: Sean Broeder
> Priority: Major
>
> The finally block in AbstractNamespace.validate() sets the status to VALID
> even when validateImpl() throws. That leaves rowType null. A later
> getRowType() on the same namespace calls validateNamespace(), which does
> nothing because the status is already VALID. validateNamespace() then calls
> getType(), which calls getRowType() again. The result is a StackOverflowError
> in place of the original error.
> For example:
> {code:java}
> select * from (select bad_col from emp) as t
> {code}
> Validating this throws "Column 'BAD_COL' not found in any table" the first
> time, as expected. But a second, independent getRowType() call on the inner
> SELECT's namespace then throws StackOverflowError instead of re-surfacing
> that error.
> Ordinary SQL through a single validate() call does not hit this. It needs a
> caller that catches the exception and keeps going, such as a validator
> extension that probes a namespace and treats failure as "no match", or code
> that asks for a namespace's type after a failed validation.
> SqlAdvisorValidator is such a caller, because it validates incomplete SQL and
> must tolerate namespaces that fail.
> Setting the status back to UNVALIDATED on failure is not enough.
> validateImpl() may already have changed shared validator state, so entering
> it a second time is unsafe. Instead, store the exception and mark the
> namespace VALID. A later getRowType() or getType() re-throws the original
> error.
> In SqlAdvisorValidator:
> - Remove the activeNamespaces guard. It treated any repeat call to
> validateNamespace() as a cycle, but AbstractNamespace already handles repeat
> calls through its own status field.
> - validateNamespace() masks CalciteException and falls back to an empty
> struct type.
> - Override inferUnknownTypes() and checkRollUp() to swallow
> CalciteException, so one unresolvable item doesn't abort validation of the
> whole enclosing namespace. SqlValidatorImpl.checkRollUp() becomes protected
> forthis.
> The namespace's cycle check throws AssertionError, which the advisor does not
> mask.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
