feat: Add MultiIndex subclass. #596
Conversation
product-auto-label
bot
added
the
api: bigquery
bigframes/core/indexes/base.py
Outdated
|
|
||
| def __init__( | ||
| # Overrided on __new__ to create subclasses like python does | ||
| def __new__( | ||
| self, |
There was a problem hiding this comment.
__new__ is a static method that takes cls as param instead of self.
| @@ -454,24 +475,27 @@ def __len__(self): | |||
| return self.shape[0] | |||
|
|
|||
|
|
|||
| # Index that mutates the originating dataframe/series | |||
| class FrameIndex(Index): | |||
There was a problem hiding this comment.
This will be a breaking change that we may not want to introduce right now. @tswast
There was a problem hiding this comment.
I am deleting a class yes, but it was more of an implementation detail of the Index class. Should only affect users if they were somehow depending on exact type or type name in their code, which would be very strange.
There was a problem hiding this comment.
Good catch @GarrettWu.
Given that we say ) -> Index: in the only method that would return such an object and we don't explicitly document FrameIndex, I'm okay overlooking this and treating FrameIndex as a private implementation detail.
bigframes/core/indexes/base.py
Outdated
| self, | ||
| data=None, | ||
| dtype=None, | ||
| *, | ||
| name=None, | ||
| session=None, | ||
| linked_frame: Union[ |
There was a problem hiding this comment.
Is it an internal only param or part of the public API? If internal, we may want to make it only in a private constructor. If public API, then should update the docs.
Looks like pandas doesn't have it.
There was a problem hiding this comment.
This is for internal purposes only. Removed from constructor, now directly set the property after initialized.
| @@ -454,24 +475,27 @@ def __len__(self): | |||
| return self.shape[0] | |||
|
|
|||
|
|
|||
| # Index that mutates the originating dataframe/series | |||
| class FrameIndex(Index): | |||
There was a problem hiding this comment.
Good catch @GarrettWu.
Given that we say ) -> Index: in the only method that would return such an object and we don't explicitly document FrameIndex, I'm okay overlooking this and treating FrameIndex as a private implementation detail.
bigframes/core/indexes/base.py
Outdated
| ): | ||
| super().__init__(series_or_dataframe._block) | ||
| self._whole_frame = series_or_dataframe | ||
| class MultiIndex(Index, vendored_pandas_multindex.MultiIndex): |
There was a problem hiding this comment.
As much as possible, I'd like our module hierarch to reflect pandas. In pandas, this is defined in pandas/core/indexes/multi.py. Anything under core is considered private-ish, but these paths do leak out in some of the pandas docs.
Let's make a bigframes/core/indexes/multi.py file and put this there.
TrevorBergeron
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕