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-12756][hive] migrate HiveCatalog from TypeInformation-based old type system to DataType-based new type system #8639
Conversation
…d type system to DataType-based new type system
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:
|
* | ||
* <p>The call order of this method determines the order of fields in the schema. | ||
*/ | ||
public Builder fields(String[] names, DataType[] dataTypes) { |
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.
It appears to me this builder isn't seem necessary or proper as the constructor does the same thing and probably in a more complete fashion.
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.
the builder is already there, and is not introduced by this PR. I feel it’s good to have this method to allow users batch loading elements with builder, which is required by this PR.
I think whether exposing the class constructor as public is orthogonal
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 meant this builder method is new! However, I didn't realize the constructor is private. With that, it's fine to have this.
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.
PR looks mostly good to me except the introduced builder.
Thanks @xuefuz for your review. Merging |
Rebased to master before merged |
…d type system to DataType-based new type system This PR migrates HiveCatalog from TypeInformation-based old type system to DataType-based new type system This closes apache#8639.
What is the purpose of the change
This PR migrates HiveCatalog from TypeInformation-based old type system to DataType-based new type system
Note: hive table InputFormat and OutputFormat may also need to migrate similarly. This PR doesn't contain those changes.
Brief change log
Verifying this change
This change is already covered by existing test
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation