-
Notifications
You must be signed in to change notification settings - Fork 62
feat: Display custom single index column in anywidget mode #2311
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
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
bigframes/display/anywidget.py
Outdated
| page_data = cached_data.iloc[start:end].copy() | ||
|
|
||
| # Handle index display | ||
| if page_data.index.name is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to check len(df.index.names) != 0 on the original BigFrames DataFrame instead? Most of the time the no-name index is going to be the default range index, but not always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like df.index.names won't work in partial ordering mode.
| @validations.requires_index |
I'll try to figure out the correct alternative and report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
df._block.has_indexThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the excellent feedback, @tswast. Your suggestion to use df._block.has_index was precisely what was needed to avoid the expensive index materialization on the full BigFrames DataFrame.
I've implemented the change to use self._dataframe._block.has_index. However, I found that an additional check was needed to fully resolve the issue and pass test_widget_with_default_index_should_display_row_column.
The full condition I've used is:
if self._dataframe._block.has_index and page_data.index.name is not None:
To clarify the inclusion of page_data.index.name is not None: this part of the check is performed on the small, local pandas page_data slice. It's a cheap operation because page_data is already in memory. Its purpose is to correctly handle the display formatting in all scenarios.
Specifically, while self._dataframe._block.has_index correctly tells us if a custom index exists on the BigFrames DataFrame, that index might still be an unnamed default (like a RangeIndex). In such cases, page_data.index.name would be None.
The combined check ensures that we only display a named custom index when both conditions are met:
- A custom index has been defined on the BigFrames DataFrame (self._dataframe._block.has_index).
- That index actually has a name (page_data.index.name is not None) to use as a header.
Otherwise, it correctly falls back to displaying the generic "Row" column. This makes the overall index display logic robust and accurate, reflecting the user's intent without triggering expensive BigQuery operations.
Let me know if this approach looks good to you.
bigframes/display/anywidget.py
Outdated
| if self._dataframe._block.has_index and page_data.index.name is not None: | ||
| # Custom named index - include it with its actual name | ||
| page_data.insert(0, page_data.index.name, page_data.index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's display the real index values, even if the index has no name.
| if self._dataframe._block.has_index and page_data.index.name is not None: | |
| # Custom named index - include it with its actual name | |
| page_data.insert(0, page_data.index.name, page_data.index) | |
| if self._dataframe._block.has_index: | |
| index_name = page_data.index.name | |
| page_data.insert(0, index_name if index_name is not None else "", page_data.index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testcase is updated accordingly
bigframes/display/anywidget.py
Outdated
| page_data = cached_data.iloc[start:end].copy() | ||
|
|
||
| # Handle index display | ||
| # TODO(b/332316283): Add tests for custom multiindex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this bug ID is incorrect. Maybe this is the correct one?
| # TODO(b/332316283): Add tests for custom multiindex | |
| # TODO(b/459515995): Add tests for custom multiindex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, I plan to use this one b/438181139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes intended? Maybe you missed my note about opting out of airlock in chat? I've also added instructions to our internal contributing docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely should not include this noxfile change. I moidfy the noxfile file locally to run nox. I will revert this file change.
tests/system/small/test_anywidget.py
Outdated
| assert "row_4" not in html # Verify it respects max_rows | ||
|
|
||
|
|
||
| # TODO(b/332316283): Add tests for custom multiindex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug ID looks incorrect here, too.
| # TODO(b/332316283): Add tests for custom multiindex | |
| # TODO(b/459515995): Add tests for custom multiindex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to use b/438181139
This PR introduces the single column display for anywidget mode.
A screenshot of single index column display is here: screen/6VLA4Nk68TsYczV
Fixes #<459515995> 🦕