Skip to content
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

Spark: Refactor SparkReadConf use primitive type for confs with default values #7429

Merged

Conversation

singhpk234
Copy link
Contributor

About Change

Address review feedback #7422 (comment) .

cc @aokolnychyi

@github-actions github-actions bot added the spark label Apr 25, 2023
Copy link
Collaborator

@szehon-ho szehon-ho left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me. But wanted to double check, is it a backward compatible change?
User will ever get NoSuchMethodError?

Copy link
Contributor

@jackye1995 jackye1995 left a 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

@jackye1995
Copy link
Contributor

jackye1995 commented Apr 27, 2023

is it a backward compatible change?

Under what case would user have this error? @szehon-ho

@singhpk234
Copy link
Contributor Author

singhpk234 commented Apr 27, 2023

we can get the values in both long, Long, so using the function call directly would not be problematic but let's say if some one is extending SparkReadConf and over-riding the streamFromTimestamp() might face issue as now the signatures don't match. But it seems like a very niche use case.

Would like to know your thoughts and make the changes accordingly, as streamFromTimestamp() is the only released public method will mark it deprecated, and add a new one with primitive type as well.

@szehon-ho
Copy link
Collaborator

Yea I am thinking, if our caller does not recompile their library on newer Iceberg version , they will hit it, but I think its not the normal flow. Thanks for double checking.

@aokolnychyi aokolnychyi merged commit faddf30 into apache:master Apr 27, 2023
@aokolnychyi
Copy link
Contributor

Thanks, @singhpk234!

@amogh-jahagirdar
Copy link
Contributor

Thanks @singhpk234 ! i was thinkiing about @szehon-ho point , I don't think we can set an expectation that people can extend SparkReadConf and have signatures align. So overall, this change looks good to me

manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023
Co-authored-by: Prashant Singh <psinghvk@amazon.com>
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.

None yet

5 participants