-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-38750][table] Validation of queries with functions erroneously invoked under SELECT fails with StackOverflow
#27287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… invoked under select fails with StackOverflow
SELECT fails with StackOverflow
SELECT fails with StackOverflowSELECT fails with StackOverflow
| } catch (ValidationException e) { | ||
| // let operand checker fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rethrowing here as CalciteContextException wrapped into ValidationException will not help for complex cases and will lead again to StackOverflow: first it rethrows as ValidationException and then swallows.
For that reason there is testNestedCoalesceOnInvalidField highlighting this case with a query
SELECT coalesce(SELECT coalesce(SELECT coalesce(invalid)))| } catch (CalciteContextException e) { | ||
| throw e; | ||
| } catch (Throwable t) { | ||
| if (t.getCause() instanceof CalciteContextException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious, do we need to check down the chain of causes for a match, rather than the first cause? I am not sure if we can get into this situation, where the cause of the cause is a CalciteContextException or the like. Maybe using this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far I'm not aware of such cases and can not imagine how I can check that it helps any query...
however even if they are present the only problem thing which might happen is a not user friendly error message since the user friendly will be inside CalciteContextException.
Thus it will not be StackOverflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking one more time here I was not able to find a test entering this code
so I removed these lines, the PR should be a bit shorter now
thanks for attracting attention to these lines
What is the purpose of the change
The PR is to fix
StackOverflowfor queries likeBrief change log
The way how
StackOverflowappears:SqlValidatorImpl#validateNamespaceSqlValidatorImpl#inferUnknownTypescoalesce) it invokes Flink'sTypeInferenceOperandInference#inferOperandTypesDelegatingScope#fullyQualifywithColumn 'invalid' not found in any tableVerifying this change
There are tests added
Does this pull request potentially affect one of the following parts:
@Public(Evolving): ( no)Documentation