Skip to content

spark 3.3 read by snapshot ref schema - #6717

Merged
aokolnychyi merged 11 commits into
apache:masterfrom
namrathamyske:spark-read-bugfix
Feb 22, 2023
Merged

spark 3.3 read by snapshot ref schema#6717
aokolnychyi merged 11 commits into
apache:masterfrom
namrathamyske:spark-read-bugfix

Conversation

@namrathamyske

@namrathamyskenamrathamyske commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

In PR #5150 we introduced read from branch or tag. There is a bug where we cannot read the snapshot specific schema changes when doing

spark.read().option("branch", <branch_name>). The fix in this PR is applied similar to #3722

Still yet to add support for tag. If given a heads up for branch, will go ahead with tag
cc @jackieo168

@rdblue@jackye1995@amogh-jahagirdar@wypoon Let me know what you think

@namrathamyskenamrathamyske changed the title spark 3.3 read by snapshot ref bug fix for schema changespark 3.3 read by snapshot ref schemaFeb 1, 2023
@namrathamyske
namrathamyske marked this pull request as ready for review February 1, 2023 06:44
Comment threadspark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java Outdated
@amogh-jahagirdar

Copy link
Copy Markdown
Contributor

Thanks for the fix @namrathamyske ! left some comments

@namrathamyske

Copy link
Copy Markdown
ContributorAuthor

Thanks @jackieo168 for coming up with this!

@jackye1995
jackye1995 self-requested a review February 6, 2023 17:45
@jackye1995

Copy link
Copy Markdown
Contributor

@aokolnychyi you might be also interested in this because #6651 depends on this

Comment threadspark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java Outdated
@namrathamyske

Copy link
Copy Markdown
ContributorAuthor

@aokolnychyi can you take a look at this ?

@amogh-jahagirdaramogh-jahagirdar left a comment

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.

Looking close just style and one bug in the SparkCachedTableCatalog implementation. Looking at CI, also like some spotless issues . Thanks @namrathamyske !

Comment threadspark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java Outdated

@jackye1995jackye1995 left a comment

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.

looks good to me!

@amogh-jahagirdaramogh-jahagirdar left a comment

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.

Just some nits overall LGTM

@jackye1995jackye1995 added this to the Iceberg 1.2.0 milestone Feb 21, 2023
Comment threadspark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java Outdated
@aokolnychyi

Copy link
Copy Markdown
Contributor

I should be able to review this one today as well.

@rdbluerdblue left a comment

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 saw a couple of nits, but overall I think this is good to go.

@jackye1995

Copy link
Copy Markdown
Contributor

I just asked for permission to push to this branch, addressing the comments

@amogh-jahagirdaramogh-jahagirdar left a comment

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.

Thanks for addressing the nits @jackye1995 , overall this looks good to be merged from my side

@aokolnychyi

Copy link
Copy Markdown
Contributor

A few minor nits and should be good to go. Thanks everyone!

@aokolnychyi
aokolnychyi merged commit ce0d075 into apache:masterFeb 22, 2023
@aokolnychyi

aokolnychyi commented Feb 22, 2023

Copy link
Copy Markdown
Contributor

Thanks, @namrathamyske and @jackye1995! Thanks for reviewing, @amogh-jahagirdar@rdblue@jackieo168!

haizhou-zhao pushed a commit to haizhou-zhao/iceberg that referenced this pull request Feb 23, 2023
krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023
Sign up for freeto 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.

6 participants

@namrathamyske@amogh-jahagirdar@jackye1995@aokolnychyi@rdblue@jackieo168