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-13304][FLINK-13322][FLINK-13323][table-runtime-blink] Fix implementation of getString and getBinary method in NestedRow, fix serializer restore in BaseArray/Map serializer and add tests for complex data formats #9139
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:
|
Travis: https://travis-ci.com/TsReaper/flink/builds/119435583 The failed python tests are not related to this PR. |
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 @tsreaper , just left a minor comment.
...ble/flink-table-runtime-blink/src/main/java/org/apache/flink/table/dataformat/NestedRow.java
Outdated
Show resolved
Hide resolved
Travis passed: https://travis-ci.com/TsReaper/flink/builds/119572124 |
LGTM +1 |
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 @tsreaper for the effort. Overall looks good to me. I left some comment regarding to the tests. Please also add some comments to the test codes to explain what's the purpose as much as possible.
...nk-table-runtime-blink/src/main/java/org/apache/flink/table/typeutils/BaseMapSerializer.java
Outdated
Show resolved
Hide resolved
...le/flink-table-runtime-blink/src/test/java/org/apache/flink/table/util/BoundaryTestUtil.java
Outdated
Show resolved
Hide resolved
...flink-table-runtime-blink/src/test/java/org/apache/flink/table/dataformat/BinaryRowTest.java
Show resolved
Hide resolved
...ble/flink-table-runtime-blink/src/main/java/org/apache/flink/table/dataformat/NestedRow.java
Show resolved
Hide resolved
...flink-table-runtime-blink/src/test/java/org/apache/flink/table/dataformat/BinaryRowTest.java
Outdated
Show resolved
Hide resolved
Travis: https://travis-ci.com/TsReaper/flink/jobs/217472587 Note that I reverted the modification about flink-mapr-fs in this travis. |
...able-runtime-blink/src/test/java/org/apache/flink/table/dataformat/DataFormatTestHelper.java
Outdated
Show resolved
Hide resolved
flink-core/src/test/java/org/apache/flink/api/common/typeutils/SerializerTestBase.java
Outdated
Show resolved
Hide resolved
…d getBinary method in NestedRow
…n BaseArray and BaseMap serializers
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 @tsreaper . LGTM.
Could you update the corresponding PR for 1.9?
The travis passed: Merging |
What is the purpose of the change
The
getString
andgetBinary
methods are not implemented correctly inNestedRow
. This PR fixes the implementation and adds test cases for complex data formats.Also, the config snapshot of BaseArray/Map serializer is not implemented correctly. When user specifies his custom kryo serializer as the element/key/value serializer and later restore the serializers from the snapshot, the element/key/value serializer may change. This PR also fixes this issue.
(This PR ought to be split into 3 PRs... I discover this problem after @wuchong reviewed this, so I'll keep this PR and let @wuchong continues his review. Sorry for the inconvenience caused)
Brief change log
getString
andgetBinary
inNestedRow
.Verifying this change
This change added tests and can be verified as follows: run the newly added unit tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation