-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[FLINK-33255] [table] Validate argument count during type inference #23520
Conversation
...ink-table-common/src/main/java/org/apache/flink/table/types/inference/TypeInferenceUtil.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/table/planner/functions/inference/TypeInferenceOperandInference.java
Outdated
Show resolved
Hide resolved
ea145d1
to
3be47e1
Compare
...able/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
Show resolved
Hide resolved
...ink-table-common/src/main/java/org/apache/flink/table/types/inference/TypeInferenceUtil.java
Show resolved
Hide resolved
.../flink-table-planner/src/main/java/org/apache/calcite/sql/validate/ExtraCalciteResource.java
Show resolved
Hide resolved
3be47e1
to
2b5453c
Compare
...able/flink-table-planner/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
Outdated
Show resolved
Hide resolved
2b5453c
to
a25f390
Compare
a25f390
to
0764aae
Compare
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.
some last suggestions from my side.
For the future. Could we create separate commits if we fix multiple issues? I could see this split into two separate commits:
- Improve exception message for signature mismatch
- Validate argument count in TypeInferenceOperandInference
...ink-table-common/src/main/java/org/apache/flink/table/types/inference/TypeInferenceUtil.java
Outdated
Show resolved
Hide resolved
.../flink-table-planner/src/main/java/org/apache/calcite/sql/validate/ExtraCalciteResource.java
Outdated
Show resolved
Hide resolved
0764aae
to
d60da2c
Compare
- Previously, TypeInferenceOperandInference was not validating argument counts which led to bugs like FLINK-33248 - This commit adds a check to validate argument count and throws an exception if the argument count doesn't match the expected number of arguments
d60da2c
to
678be13
Compare
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.
LGTM. Thank you for this really nice improvement @bvarghese1 ! I think caring about exception messages is often underappreciated.
What is the purpose of the change
Validate argument count during type inference to avoid bugs like [FLINK-33248]
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation