Flink: Support writes to branches in FlinkSink #6660

amogh-jahagirdar · 2023-01-25T00:15:20Z

This change adds support to write to branches in Flink Sink via a FlinkWriteOption "branch"

amogh-jahagirdar · 2023-01-25T16:01:46Z

moving this out of draft.

Beyond the code wanted to discuss high level API design for branch writes using FlinkSink. In the current implementation we're using a FlinkWriteOption so it looks like this right now:

FlinkSink sink = FlinkSink.forRow(dataStream, SimpleDataUtil.FLINK_SCHEMA)
    .table(table)
    .tableLoader(tableLoader)
    .set("write-format", "avro")
    .set(FlinkWriteOptions.BRANCH, "some-branch")
    .set(FlinkWriteOptions.OVERWRITE_MODE, "true")

As I implemented this though started to think we may just want to have a direct branch method on the FlinkSink builder itself. That seems more intuitive from an API perspective and is just easier to use. The API below seems better to me

FlinkSink sink = FlinkSink.forRow(dataStream, SimpleDataUtil.FLINK_SCHEMA)
    .table(table)
    .tableLoader(tableLoader)
    .branch("some-branch")
    .set("write-format", "avro")
    .set(FlinkWriteOptions.OVERWRITE_MODE, "true")

@stevenzwu @yyanyy @rdblue @jackye1995 @namrathamyske Let me know your thoughts here!

amogh-jahagirdar · 2023-01-25T17:13:36Z

Need to fix up the tests also

stevenzwu · 2023-01-25T18:28:46Z

As I implemented this though started to think we may just want to have a direct branch method on the FlinkSink builder itself. That seems more intuitive from an API perspective and is just easier to use.

Yeah. I also think an explicit toBranch method in the builder itself is more intuitive.

flink/v1.16/flink/src/main/java/org/apache/iceberg/flink/sink/IcebergFilesCommitter.java

flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java

jackye1995 · 2023-01-25T21:54:14Z

As I implemented this though started to think we may just want to have a direct branch method on the FlinkSink builder itself. That seems more intuitive from an API perspective and is just easier to use.

I think this is more of a question about convention, I will leave to people who use Flink more often to have an opinion. To me use toBranch definitely feels cleaner.

jackye1995

looks good to me, thanks for addressing the comments!

amogh-jahagirdar · 2023-01-26T00:06:57Z

Thanks for the reviews @stevenzwu @jackye1995 !

flink/v1.16/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java

pvary · 2023-01-26T06:21:59Z

Thanks for the PR @amogh-jahagirdar!
Left one small question.

Also, do we have this feature in FlinkSource?

amogh-jahagirdar · 2023-01-26T21:02:24Z

Thanks for the PR @amogh-jahagirdar! Left one small question.

Also, do we have this feature in FlinkSource?

Thanks for the review @pvary ! Not yet, but I'm working on a PR for it, will publish when it's ready .

flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSinkV2.java

hililiwei · 2023-01-30T11:51:58Z

Thanks for the PR @amogh-jahagirdar! Left one small question.
Also, do we have this feature in FlinkSource?

Thanks for the review @pvary ! Not yet, but I'm working on a PR for it, will publish when it's ready .

A long time ago, I raised a related #5029 to do this, if you can, can you take a look?

jackye1995 · 2023-01-30T22:30:51Z

A long time ago, I raised a related #5029 to do this, if you can, can you take a look?

Thanks, marking that as a part of the milestone!

flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSinkBranch.java

jackye1995 · 2023-02-12T17:55:48Z

flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/sink/TestIcebergFilesCommitter.java

-      new Object[] {"parquet", 2},
-      new Object[] {"orc", 1},
-      new Object[] {"orc", 2}
+      new Object[] {"avro", 1, "main"},


nit: can we use SnapshotRef.MAIN_BRANCH for all the places we have "main" hard-coded?

Also similar to the comment in Spark https://github.com/apache/iceberg/pull/6651/files#r1101939155, can we reduce the number of combinations here?

stevenzwu · 2023-02-13T04:25:17Z

thanks @amogh-jahagirdar for the contribution and @jackye1995 and @hililiwei for review.

amogh-jahagirdar · 2023-02-14T05:10:35Z

Thanks for the reviews! @stevenzwu @jackye1995 @hililiwei

github-actions bot added the flink label Jan 25, 2023

jackye1995 self-requested a review January 25, 2023 05:29

amogh-jahagirdar force-pushed the flink-branch-writes branch 7 times, most recently from b682cbe to 1ee27ed Compare January 25, 2023 15:55

amogh-jahagirdar marked this pull request as ready for review January 25, 2023 16:02

amogh-jahagirdar changed the title ~~Flink: Support writes to branches~~ Flink: Support writes to branches in FlinkSink Jan 25, 2023

amogh-jahagirdar force-pushed the flink-branch-writes branch 2 times, most recently from 6be254d to 9fe7b8b Compare January 25, 2023 17:53

stevenzwu reviewed Jan 25, 2023

View reviewed changes

flink/v1.16/flink/src/main/java/org/apache/iceberg/flink/sink/IcebergFilesCommitter.java Show resolved Hide resolved

amogh-jahagirdar force-pushed the flink-branch-writes branch 2 times, most recently from 1ceaf7f to 7456b04 Compare January 25, 2023 21:34

jackye1995 reviewed Jan 25, 2023

View reviewed changes

flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java Show resolved Hide resolved

amogh-jahagirdar force-pushed the flink-branch-writes branch 3 times, most recently from c1d0dd6 to 855fb9d Compare January 25, 2023 22:58

jackye1995 approved these changes Jan 25, 2023

View reviewed changes

amogh-jahagirdar requested a review from stevenzwu January 26, 2023 03:14

pvary reviewed Jan 26, 2023

View reviewed changes

flink/v1.16/flink/src/main/java/org/apache/iceberg/flink/sink/FlinkSink.java Show resolved Hide resolved

amogh-jahagirdar force-pushed the flink-branch-writes branch from 0461c9a to 35e2fca Compare January 27, 2023 22:50

amogh-jahagirdar commented Jan 27, 2023

View reviewed changes

flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSinkV2.java Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the flink-branch-writes branch 3 times, most recently from f9a8f60 to ca5ddd0 Compare January 28, 2023 01:17

hililiwei approved these changes Jan 30, 2023

View reviewed changes

stevenzwu reviewed Jan 31, 2023

View reviewed changes

jackye1995 mentioned this pull request Feb 3, 2023

Flink: Use Tag or Branch to scan data. #5029

Merged

amogh-jahagirdar mentioned this pull request Feb 7, 2023

Spark 3.3 write to branch snapshot #6651

Closed

amogh-jahagirdar force-pushed the flink-branch-writes branch 3 times, most recently from 86573fb to 9a78a9d Compare February 8, 2023 05:52

Flink: Support branch writes in Flink sink

1bfe2b3

amogh-jahagirdar force-pushed the flink-branch-writes branch from 9a78a9d to 1bfe2b3 Compare February 8, 2023 06:18

amogh-jahagirdar closed this Feb 8, 2023

amogh-jahagirdar reopened this Feb 8, 2023

amogh-jahagirdar requested review from stevenzwu and removed request for pvary February 9, 2023 15:47

jackye1995 reviewed Feb 12, 2023

View reviewed changes

stevenzwu approved these changes Feb 13, 2023

View reviewed changes

stevenzwu merged commit a7a6d35 into apache:master Feb 13, 2023

amogh-jahagirdar mentioned this pull request Feb 27, 2023

Flink: Add branch to successful commit logs #6950

Merged

stevenzwu mentioned this pull request Feb 28, 2023

Flink: fix compiling error from PR #6753 #6953

Merged

stevenzwu pushed a commit that referenced this pull request Feb 28, 2023

Flink: Backport PR #6660 to Flink 1.14 and 1.15 (#6949)

d1679df

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

Flink: Support branch writes in Flink sink (apache#6660)

0dcf316

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

Flink: Backport PR apache#6660 to Flink 1.14 and 1.15 (apache#6949)

2371755

Flink: Support writes to branches in FlinkSink #6660

Flink: Support writes to branches in FlinkSink #6660

Uh oh!

Conversation

amogh-jahagirdar commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amogh-jahagirdar commented Jan 25, 2023

Uh oh!

stevenzwu commented Jan 25, 2023

Uh oh!

Uh oh!

Uh oh!

jackye1995 commented Jan 25, 2023

Uh oh!

jackye1995 left a comment

Choose a reason for hiding this comment

Uh oh!

amogh-jahagirdar commented Jan 26, 2023

Uh oh!

Uh oh!

pvary commented Jan 26, 2023

Uh oh!

amogh-jahagirdar commented Jan 26, 2023

Uh oh!

Uh oh!

hililiwei commented Jan 30, 2023

Uh oh!

jackye1995 commented Jan 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackye1995 Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

jackye1995 Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

stevenzwu commented Feb 13, 2023

Uh oh!

amogh-jahagirdar commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

amogh-jahagirdar commented Jan 25, 2023 •

edited

Loading

amogh-jahagirdar commented Jan 25, 2023 •

edited

Loading