feat: Add warning when user tries to access struct series fields with __getitem__
#1082
Conversation
product-auto-label
bot
added
size: m
| with pytest.warns(UserWarning, match=r"Series\.struct\.field\(.+\)"): | ||
| s[key] | ||
| else: | ||
| s[key] |
There was a problem hiding this comment.
Should we have a check that a warning is not raised in this case?
I would prefer two separate tests to make this easier to follow. See go/tott/661
There was a problem hiding this comment.
I separated the tests into two parts: 1 for warning and 1 for no warning. Hopefully this makes it easier to read and understand.
| session=session, | ||
| ) | ||
|
|
||
| s[key] |
There was a problem hiding this comment.
Please make this test fail if a warning is raised.
| session=session, | ||
| ) | ||
|
|
||
| s[key] |
There was a problem hiding this comment.
Please make this test fail if a warning is raised.
There was a problem hiding this comment.
Thanks! Maybe worth a comment about the filterwarnings above, as it'n not 100% obvious to me.
Alternatively, could we use the solution suggested here, instead?
| s[key] | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("error", category=UserWarning) | |
| s[key] |
Better yet, have a custom category so we know precisely what warning to fail on.
There was a problem hiding this comment.
Yeah this approach is simpler. Defined a dedicated warning class instead. I also set a stack level as suggested.
bigframes/core/indexers.py
Outdated
|
|
||
| if not bigframes.dtypes.is_string_like(series.index.dtype): | ||
| warnings.warn( | ||
| "Are you trying to access struct fields? If so, please use Series.struct.field(...) method instead." |
There was a problem hiding this comment.
Could we do a custom category (defined in bigframes.exceptions) here to make it a bit easier to filter out if needed for some reason?
Also, I recommend setting stacklevel so that the error shows up in user code, not as appearing deep in BigFrames.
There was a problem hiding this comment.
I have provided a stack level such that the warning call site is now at __getitem__
| session=session, | ||
| ) | ||
|
|
||
| s[key] |
There was a problem hiding this comment.
Thanks! Maybe worth a comment about the filterwarnings above, as it'n not 100% obvious to me.
Alternatively, could we use the solution suggested here, instead?
| s[key] | |
| with warnings.catch_warnings(): | |
| warnings.simplefilter("error", category=UserWarning) | |
| s[key] |
Better yet, have a custom category so we know precisely what warning to fail on.
No description provided.