Skip to content

Remove cached properties before updating table scans #2178

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

smaheshwar-pltr
Copy link
Contributor

@smaheshwar-pltr smaheshwar-pltr commented Jul 7, 2025

Rationale for this change

Closes #2179.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, the scenario shown in the test and described in the issue now works.

return type(self)(**{**self.__dict__, **overrides})
data = {**self.__dict__, **overrides}

# Cached properties are also stored in the __dict__, so must be removed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that normal methods annotated with property (not cached_property) aren't stored in dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is the nicest solution, though it feels like a minimally viable one

@smaheshwar-pltr smaheshwar-pltr marked this pull request as ready for review July 7, 2025 15:25
@jayceslesar
Copy link
Contributor

you could add **kwargs in the TableScan constructor too right as a way to "fix"? I dont really think there is a good way to deal with this hahahaha

@smaheshwar-pltr
Copy link
Contributor Author

you could add **kwargs in the TableScan constructor too right as a way to "fix"?

Yes, I think you're right.

I was a bit hesitant to change the constructor, and also technically subclasses of it would need to take **kwargs in their constructors too to avoid this issue themselves which feels easy to miss (though admittedly without #2031 we only have DataScan subclassing). But inspection doesn't feel nice either 🤔

@jayceslesar
Copy link
Contributor

Maybe its fine to duplicate the update method in the subclasses and leave a comment about why its there for the cached properties?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] scan.filter after reading it as an Arrow table throws
2 participants