Skip to content

Flink: switch to FileScanTaskParser for JSON serialization of IcebergSourceSplit#7978

Merged
stevenzwu merged 3 commits intoapache:masterfrom
stevenzwu:issue-1698
Jul 8, 2023
Merged

Flink: switch to FileScanTaskParser for JSON serialization of IcebergSourceSplit#7978
stevenzwu merged 3 commits intoapache:masterfrom
stevenzwu:issue-1698

Conversation

@stevenzwu
Copy link
Copy Markdown
Contributor

No description provided.

@stevenzwu stevenzwu requested review from Fokko, nastra and pvary July 4, 2023 02:13
@github-actions github-actions Bot added the flink label Jul 4, 2023
Copy link
Copy Markdown
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I left a comment around error msg clarity


Preconditions.checkArgument(
writtenTasks == fileScanTasks.size(),
"Invalid combined scan task: collection size = %s, iterator size = %s",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's clear to a reader what the difference between collection size and iterator size is

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally agree. removed this unnecessary check and simplified the loop.

stevenzwu and others added 2 commits July 7, 2023 19:59
…e/split/IcebergSourceSplit.java

Co-authored-by: Eduard Tudenhoefner <etudenhoefner@gmail.com>
@stevenzwu stevenzwu merged commit 623b9b7 into apache:master Jul 8, 2023
@stevenzwu
Copy link
Copy Markdown
Contributor Author

issue #1698

@stevenzwu
Copy link
Copy Markdown
Contributor Author

thanks @nastra and @pvary for the review

stevenzwu added a commit to stevenzwu/iceberg that referenced this pull request Aug 4, 2023
stevenzwu added a commit that referenced this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants