Fix TableScan.update to exclude cached properties

[smaheshwar-pltr]

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.

[smaheshwar-pltr]

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

[smaheshwar-pltr]

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

[smaheshwar-pltr]

See #2178 (comment), I'm no longer convinced

[jayceslesar]

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]

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]

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

[smaheshwar-pltr]

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

Yeah I think maybe we should just make a larger change here and having subclasses implementing updates sounds good. Will need to think about how to go about that though

I realised that the current PR (that removes only cached properties) doesn't guard against the case where a subclass has a private member.

[kevinjqliu]

interesting! good catch.

I think implicitly the update function only allows updating the constructor parameters. the cached_property is a side-effect of using self.__dict__

what if we just filter for only the constructor params? wdyt?

import inspect

def update(self: S, **overrides: Any) -> S:
    """Create a copy of this table scan with updated fields."""
    init_params = inspect.signature(type(self).__init__).parameters
    valid_keys = set(init_params) - {"self"}
    filtered_dict = {k: v for k, v in self.__dict__.items() if k in valid_keys}
    return type(self)(**{**filtered_dict, **overrides})

[smaheshwar-pltr]

what if we just filter for only the constructor params? wdyt?

Yes, I thought about this and think it's a good idea! We achieve something similar to that too with @jayceslesar's **kwargs idea that I think is also worth considering.

Was also toying with a polymorphic solution (#2199) that doesn't assume that init parameters and members are named the same for TableScan's subclasses. (Hat-tip to #2178 (comment))

[smaheshwar-pltr]

After some thought, we can maybe narrow down to a few approaches:

1. Inspect constructor of subclass and match attribute names to parameter names; I've implemented this here now (9edb166), in the spirit of Fix TableScan.update to exclude cached properties #2178 (comment)
2. Lift the attribute name matching assumption by introducing a separate arguments property that subclasses can override: [Draft] Fix TableScan updating with arguments method #2199
3. Use **kwargs as in Fix TableScan.update to exclude cached properties #2178 (comment) so the constructor ignores unrecognised arguments
4. Spitballing: a larger change to the design to make update more fluid (when reading the code my mind goes to https://docs.python.org/3/library/dataclasses.html#dataclasses.replace), though might be hard and even harder to do so in a non-breaking way.

@kevinjqliu @jayceslesar, happy to take suggestions / hear thoughts! The first three work; I'm leaning towards (1) currently.

[smaheshwar-pltr]

@kevinjqliu, I wrote things slightly differently to #2178 (comment), LMKWYT of this.

Preferred getattr over self.__dict__ since it feels less low-level.

[kevinjqliu]

LGTM! this is great. lets add some comments to explain why we're doing this

[smaheshwar-pltr]

Good point! 776c433

[kevinjqliu]

Thanks for the thoughtful explanation @jayceslesar

I prefer option 1 too. We're making the intent of **self.__dict__ more explicit by only using the parameters of __init__.

Option 2 introduces some maintenance burden; we'd have to remember updating _arguments whenever a new param is added to __init__.
As you pointed out, option 3 requires subclasses to follow the same signature. It might be bypassed by subclasses if **kwargs is not specified
Option 4 is interesting. Pydantic's model_dump by default ignored property

[kevinjqliu]

Im in favor of option 1, the current approach
Looks like theres a merge conflict too

[kevinjqliu]

LGTM! this is great. lets add some comments to explain why we're doing this

[smaheshwar-pltr]

@kevinjqliu, merged and added comment! Are we good to merge?

[kevinjqliu]

Thanks for adding a fix this bug @smaheshwar-pltr. And thanks for the great discussion and review @jayceslesar