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-13286][table-api] Port connector related validators to api-java-bridge #9168
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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.
thanks for this pr @JingsongLi , LGTM (left a minor comment)
// check the environment | ||
if (!isStreamEnvironment) { | ||
throw new ValidationException( | ||
format("Property %s' is not allowed in a batch environment.", proctime)); |
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.
nite: add '
before %s
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.
nice catch
Hi @JingsongLi , I think we should also move the corresponding tests to table-api or table-common, because they do not depend on planner. What do you think?
|
Others looks good to me. |
I think we can move java test, and scala test could be port to java in another commit. |
Good suggestion, and previous |
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 had one minor comment. Otherwise +1. We haven't ported many tests yet. So its fine to do that in follow up PRs.
clazz); | ||
break; | ||
default: | ||
throw new RuntimeException("Unsupported rowtime timestamps type: " + t.get()); |
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.
nit: use ValidationException
here and at other places where the user did something wrong
The travis is passed: https://travis-ci.com/flink-ci/flink/builds/119766173. |
… api-java-bridge This closes #9168
What is the purpose of the change
Port these class to java from scala. And remove scala apply method.
Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation