[query] fix shadowing of field names by methods - #13498
Conversation
In particular, struct field names which clash with methods on `StructExpression`.
|
@danking This doesn't forbid clashing field names, but accessing the field must use the |
|
Hmm. This also seems fine. I'm gonna assign Chris for a third perspective. |
chrisvittal
left a comment
chrisvittal
left a comment
There was a problem hiding this comment.
LGTM. Just need to fix the parts of the tests where we overwrote items as well.
|
Ugh, approx_cdf return a struct with a "values" field. No matter what we do to fix the shadowing issue is going to be a breaking change. |
|
Does |
|
The sampled values. The method is shadowed and inaccesible. |
patrick-schultz
left a comment
patrick-schultz
left a comment
There was a problem hiding this comment.
Note that this fixes the behavior to aggree with the existing docs on StructExpression:
However, it is recommended to use square brackets to select fields:
>>> hl.eval(struct['a'])
5The latter syntax is safer, because fields that share their name with
an existing attribute of :class:.StructExpression(keys,values,
annotate,drop, etc.) will only be accessible using the
:meth:.StructExpression.__getitem__syntax. This is also the only way
to access fields that are not valid Python identifiers, like fields with
spaces or symbols.
| super(StructExpression, s).__init__(x, t, indices, aggregations) | ||
| s._warn_on_shadowed_name = set() | ||
| s._fields = {} | ||
| for k, v in fields.items(): | ||
| s._set_field(k, v) | ||
| super(StructExpression, s).__init__(x, t, indices, aggregations) | ||
| return s |
There was a problem hiding this comment.
Initializing the superclass first lets us catch shadowing superclass attributes
| def __getattribute__(self, item): | ||
| if item in super().__getattribute__('_warn_on_shadowed_name'): | ||
| warning(f'Field {item} is shadowed by another method or attribute. ' | ||
| f'Use ["{item}"] syntax to access the field.') | ||
| self._warn_on_shadowed_name.remove(item) | ||
| return super().__getattribute__(item) | ||
|
|
There was a problem hiding this comment.
__getattribute__ is always called on any attribute lookup, whereas __getattr__ is only called as a last resort when the attribute isn't found in any of the standard places. We need the first to catch uses of an attribute which is both a field name and an existing attribute.
| if item in self.__dict__: | ||
| return self.__dict__[item] |
There was a problem hiding this comment.
Looking in __dict__ was redundant. If the attribute is in __dict__, the interpreter will find it and not call __getattr__.
| if key in self.__dict__ or hasattr(super(), key): | ||
| self._warn_on_shadowed_name.add(key) | ||
| else: | ||
| self.__dict__[key] = value |
There was a problem hiding this comment.
Catching shadowed fields would be easier if we didn't add fields to __dict__, but we need to do this to expose fields to IDEs for autocomplete.
|
Chris is out, I'll take this. |
danking
left a comment
danking
left a comment
There was a problem hiding this comment.
This is excellent! Can you edit the PR message to include a CHANGELOG: that describes this change to the scientists, including a note about the breaking change (which is OK b/c bug fix).
| raise KeyError(get_nice_field_error(self, item)) | ||
|
|
||
| def __getattribute__(self, item): | ||
| if item in super().__getattribute__('_warn_on_shadowed_name'): |
There was a problem hiding this comment.
This avoids infinite recursion, right? And the super's getattribute is able to see the subclasses basic fields?
There was a problem hiding this comment.
Yes, and yes. The super's getattribute is just what would have been called if we didn't override it (unless there's some special-method magic I don't understand).
3 participants
In particular, struct field names which clash with methods on
StructExpression.close #13495
CHANGELOG: Fix a bug where field names can shadow methods on the StructExpression class, e.g. "items", "keys", "values". Now the only way to access such fields is through the getitem syntax, e.g. "some_struct['items']". It's possible this could break existing code that uses such field names.