-
Notifications
You must be signed in to change notification settings - Fork 2
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
Attribution for Spark ( Iceberg ) Support RC1 #18
Conversation
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've left some comments, almost approvable (though I must admit I assumed that the docker and some of the int test bits were just a copy from the other PRs), I would also add to these that the original PR request raised a few months ago has other suggestions for change I did not see below so worth to double check why it was an issue there but not in this case.
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
Description
What type of PR is this? (check all applicable)
Related Tickets & Documents
Checklist
Added tests?
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?