-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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-23537][table-common] Make JoinedRowData public #16631
Conversation
* | ||
* <p>This implementation is mutable to allow for performant changes in hot code paths. | ||
*/ | ||
@PublicEvolving | ||
public class JoinedRowData implements RowData { | ||
|
||
private RowKind rowKind = RowKind.INSERT; | ||
private RowData row1; |
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 didn't add @Nullable
to the fields since this produces a lot of warnings in the class, and making the IDE happy by checking non-nullability first imposes a runtime cost. I decided to instead document this explicitly.)
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 4829e0b (Thu Sep 23 18:53:04 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. 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:
|
@@ -27,7 +27,7 @@ | |||
import org.apache.flink.table.data.RowData; | |||
import org.apache.flink.table.data.StringData; | |||
import org.apache.flink.table.data.TimestampData; | |||
import org.apache.flink.table.data.utils.JoinedRowData; | |||
import org.apache.flink.table.data.JoinedRowData; |
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.
Personally, I don't suggest to move package.
- I believe there are some external connectors are aready using it and this will break them.
- It was moved from
org.apache.flink.table.data
toorg.apache.flink.table.data.utils
in [FLINK-18858][connector-kinesis] Add Kinesis sources and sinks #13770 (comment).
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 Jark, that's a good point. I'll just remove the second commit.
assertEquals(1L, joinedRow.getLong(0)); | ||
assertEquals(2L, joinedRow.getLong(1)); | ||
assertEquals(3L, joinedRow.getLong(2)); | ||
assertEquals("4", joinedRow.getString(3).toString()); |
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.
Extend this test case to also test the replace
method. It's probably the most important method in this class.
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.
Fair point! I've added a test case for it.
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.
Looks good to me. Let's see if Jark has some other thoughts.
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.
What is the purpose of the change
This exposes
JoinedRowData
as public API. When implementing Table API connectors, the need to joinRowData
arises naturally from having to append metadata to the produced physical row. We should therefore make it easy for implementors to do so without relying on internal APIs.This PR makes the class public, updates its documentation and adds some basic tests for it.
Verifying this change
This change is already covered by existing tests. However,
JoinedRowDataTest
has also been added.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation