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.

Sign up for GitHub

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

Jump to bottom

Core: Make TableScanContext immutable #5985

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Core: Make TableScanContext immutable #5985

merged 1 commit into from
Apr 27, 2023

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Oct 14, 2022 •
edited

This is a shorter alternative to #5982

@nastra nastra force-pushed the immutable-table-scan-context branch from 31aaad0 to e1ab18d Compare October 14, 2022 11:38
@nastra nastra changed the title Core: Optimize code in TableScanContext Core: Make TableScanContext immutable Oct 14, 2022
@github-actions github-actions bot added the core label Oct 14, 2022
@nastra nastra force-pushed the immutable-table-scan-context branch from e1ab18d to 4d478d4 Compare October 14, 2022 11:44
XBaith
XBaith reviewed Oct 14, 2022
View reviewed changes
Copy link

@XBaith XBaith left a comment

Choose a reason for hiding this comment

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

looks great!

@hililiwei hililiwei mentioned this pull request Oct 17, 2022
Closed
@zinking
Copy link
Contributor

zinking commented Oct 18, 2022

+1 for the refactoring refactoring.
is builder also generated by the annotation?

@nastra
Copy link
Contributor Author

nastra commented Oct 18, 2022

+1 for the refactoring refactoring. is builder also generated by the annotation?

yes the Builder is also being generated by the Immutables library

@nastra nastra requested a review from rdblue October 18, 2022 10:27
@nastra nastra mentioned this pull request Oct 19, 2022
Merged
Fokko
Fokko approved these changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

I like it, makes it much more concise and easier to read

@nastra nastra force-pushed the immutable-table-scan-context branch from 4d478d4 to 3bd5705 Compare March 23, 2023 09:13
@nastra nastra force-pushed the immutable-table-scan-context branch from 3bd5705 to f5f0463 Compare April 26, 2023 11:40
jackye1995
jackye1995 approved these changes Apr 27, 2023
View reviewed changes
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

I will go ahead to merge this to unblock #5984, and mark Liwei as coauthor to close #5982. Thanks everyone!

@jackye1995 jackye1995 merged commit 882459d into apache:master Apr 27, 2023
41 checks passed
@nastra nastra deleted the immutable-table-scan-context branch April 27, 2023 05:19
coufon pushed a commit to coufon/iceberg that referenced this pull request Apr 28, 2023
@nastra @hililiwei @coufon
Core: Make TableScanContext immutable (apache#5985)
72750c6 
Co-authored-by: Liwei Li <hililiwei@gmail.com>
manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023
@nastra @hililiwei @manisin
Core: Make TableScanContext immutable (apache#5985)
b18cbe9 
Co-authored-by: Liwei Li <hililiwei@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fokko Fokko Fokko approved these changes

@jackye1995 jackye1995 jackye1995 approved these changes

@XBaith XBaith XBaith approved these changes

@rdblue rdblue Awaiting requested review from rdblue

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nastra @zinking @jackye1995 @Fokko @XBaith