-
Notifications
You must be signed in to change notification settings - Fork 62
fix: Improve Anywidget pagination and display for unknown row counts #2258
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
fix: Improve Anywidget pagination and display for unknown row counts #2258
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
bigframes/display/anywidget.py
Outdated
| # If row count is unknown, allow any non-negative page | ||
| if self.row_count is None: | ||
| return max(0, value) |
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.
What are the cases where we would expect to get a negative value?
I suspect we may want to raise an exception because it indicates some bad state has been entered. Better to show an error than to continue in a bad state.
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.
It is a safe guard, when system has a glitches. I agree with you, I change to an error catch for negative numbers.
bigframes/display/anywidget.py
Outdated
| # there are multiple pages and show "page 1 of many" in this case | ||
| self._reset_batches_for_new_page_size() | ||
| if self._batches is None or self._batches.total_rows is None: | ||
| # TODO(b/428238610): We could still end up with a None here if the |
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.
This bug is marked as fixed. Is there other context for what action is needed? Also, as a TODO, I'm not sure what the intended update to the code would be. Is this just supposed to be a comment?
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 have removed TODO. The code is updated to differentiate between a failure to get any batches at all and successfully getting batches (a true error) but not having a known total row count (an expected condition for the "page X of many" display).
bigframes/display/anywidget.py
Outdated
| if self._batches is None or self._batches.total_rows is None: | ||
| # TODO(b/428238610): We could still end up with a None here if the | ||
| # underlying execution doesn't produce a total row count. | ||
| self._error_message = "Could not determine total row count. Data might be unavailable or an error occurred." |
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.
Could we separate total rows and batches None? As stated in the comment above none for total_rows is expected, not an error.
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.
This is a good point. I have updated the logic to separate these tow conditions.
The code now sets an _error_message if _batches itself is None, which indicates a true failure to retrieve data. The case where _batches.total_rows is None is not handled as the expected state for an unknown row count, and no error message is generated. This aligns with your suggestion.
|
@tswast Upon checking, the failed e2e tests are not related to my change. |
tswast
left a comment
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.
Thank you!
PR created by the Librarian CLI to initialize a release. Merging this PR will auto trigger a release. Librarian Version: v0.7.0 Language Image: us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/python-librarian-generator@sha256:c8612d3fffb3f6a32353b2d1abd16b61e87811866f7ec9d65b59b02eb452a620 <details><summary>bigframes: 2.30.0</summary> ## [2.30.0](v2.29.0...v2.30.0) (2025-12-03) ### Features * Support mixed scalar-analytic expressions (#2239) ([20ab469](20ab469d)) * Allow drop_duplicates over unordered dataframe (#2303) ([52665fa](52665fa5)) * Preserve source names better for more readable sql (#2243) ([64995d6](64995d65)) * use end user credentials for `bigframes.bigquery.ai` functions when `connection_id` is not present (#2272) ([7c062a6](7c062a68)) * pivot_table supports fill_value arg (#2257) ([8f490e6](8f490e68)) * Support builtins funcs for df.agg (#2256) ([956a5b0](956a5b00)) * add bigquery.json_keys (#2286) ([b487cf1](b487cf1f)) * Add agg/aggregate methods to windows (#2288) ([c4cb39d](c4cb39dc)) * Add bigframes.pandas.crosstab (#2231) ([c62e553](c62e5535)) * Implement single-column sorting for interactive table widget (#2255) ([d1ecc61](d1ecc61b)) ### Bug Fixes * Pass credentials properly for read api instantiation (#2280) ([3e3fe25](3e3fe259)) * Update max_instances default to reflect actual value (#2302) ([4489687](4489687e)) * Improve Anywidget pagination and display for unknown row counts (#2258) ([508deae](508deae5)) * Fix issue with stream upload batch size upload limit (#2290) ([6cdf64b](6cdf64b0)) * calling info() on empty dataframes no longer leads to errors (#2267) ([95a83f7](95a83f77)) * do not warn with DefaultIndexWarning in partial ordering mode (#2230) ([cc2dbae](cc2dbae6)) ### Documentation * update docs and tests for Gemini 2.5 models (#2279) ([08c0c0c](08c0c0c8)) * Add Google Analytics configuration to conf.py (#2301) ([0b266da](0b266da1)) * fix LogisticRegression docs rendering (#2295) ([32e5313](32e53134)) * update API reference to new `dataframes.bigquery.dev` location (#2293) ([da06439](da064397)) * use autosummary to split documentation pages (#2251) ([f7fd2d2](f7fd2d20)) </details>
Previously, when the total number of rows (row_count) was unknown (e.g., due to deferred computation or errors), it would incorrectly default to 0. This resulted in confusing UI, such as displaying "Page 1 of 0", and allowed users to navigate to empty pages without automatically returning to valid data.
current display strategy for the interactive table widget:
When
row_countis a positive number (e.g., 50):When
row_countisNone(unknown):Fixes #<452747934> 🦕