-
Notifications
You must be signed in to change notification settings - Fork 63
feat: Implement single-column sorting for interactive table widget #2255
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
base: main
Are you sure you want to change the base?
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
bigframes/display/anywidget.py
Outdated
| except KeyError: | ||
| logging.warning( | ||
| f"Attempted to sort by unknown column: {self.sort_column}" | ||
| ) |
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.
Why are we catching this exception and dropping it? If we get to this state something bad has happened and the user should know about it.
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.
Here is my plan:
When a KeyError is caught, the current implementation does the following:
- Notifies the user: It sets self._error_message to a user-friendly message like "Column '...' not found.". The frontend will then display this error.
- Reverts to unsorted: It resets self.sort_column = "".
- Displays the unsorted table: The function then continues, but since sort_column is now empty, it fetches and displays the data in its original, unsorted order.
bigframes/display/table_widget.js
Outdated
| const headers = tableContainer.querySelectorAll("th"); | ||
| headers.forEach((header) => { | ||
| const columnName = header.textContent.trim(); | ||
| if (columnName) { |
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.
Not all data types are sortable. See the orderable property in our dtypes.
| orderable: bool = False |
We should not add the handler or arrow to columns that we can't sort.
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.
Here is the plan:
- Display a dot (●) indicator for all sortable columns by default
- Allow users to cycle through three states: unsorted (●) → ascending (▲) → descending (▼) → unsorted (●)
- Only show sorting UI for columns with orderable data types
|
@tswast Upon checking, the failed e2e tests |
| allow_none=True, | ||
| ).tag(sync=True) | ||
| table_html = traitlets.Unicode().tag(sync=True) | ||
| sort_column = traitlets.Unicode("").tag(sync=True) |
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.
Have we considered having multiple columns as a possibility? I think a single column is a good starting point, but I think it's an alternative worth considering, especially when a particular column contains lots of duplicate values, like a "date" column.
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 agree that multi-column sorting is particularly valuable when a column has many duplicate values. I would like to get the single column sorting checked in first as a PR. Then check in a second PR for multi-column sorting. This current PR is already complex enough. I prefer two separate PRs as enhancements.
bigframes/display/anywidget.py
Outdated
| self._all_data_loaded = False | ||
| self._batch_iter: Optional[Iterator[pd.DataFrame]] = None | ||
| self._cached_batches: List[pd.DataFrame] = [] | ||
| self._last_sort_state: Optional[Tuple[str, bool]] = 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.
I can guess based on context what these mean, but I think a frozen dataclass would be much easier to understand at a glance.
bigframes/display/anywidget.py
Outdated
| self.page_size = initial_page_size | ||
| self.orderable_columns = [ | ||
| col | ||
| for col in dataframe.columns |
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.
We should iterate through dtypes directly to get both the type and the column name in one call.
Calling .dtypes for each iteration is making this into an O(n^2) algorithm. The n could be as large as 10,000, where that would definitely make a difference, though I imagine our widget would break well before then.
Aside: could you file an issue to check our limitations on the number of columns from a ux perspective? I imagine we'll need to solve that at some point soon-ish.
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.
Complexity is reduced based on the suggestion
filed b/462525985 as TODO
bigframes/display/anywidget.py
Outdated
| df_to_display = df_to_display.sort_values( | ||
| by=self.sort_column, ascending=self.sort_ascending | ||
| ) | ||
| except KeyError: |
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'm afraid of this. KeyError is a relatively common error in Python. Just looking at this code, it's not clear to me that this would always be the case of a missing column. Please check for the column presence explicitly, instead.
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 should trigger this error to be honest. The user only allows the click on a sortable column name to trigger this part of code. Theoretically, we do not even need to catch this bug, because this exception should never be hit in our design. I will remove this try catch block.
|
|
||
| // Add click handlers to column headers for sorting | ||
| const headers = tableContainer.querySelectorAll("th"); | ||
| headers.forEach((header) => { |
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 quite a bit of logic. I would like to get JavaScript-level unit tests setup before merging this PR.
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've set up a comprehensive JavaScript unit testing environment and implemented tests for the TableWidget's interactive sorting functionality.
Testing Framework & Environment:
* Jest: Chosen as the primary testing framework due to its all-in-one nature (runner, assertion library, mocking) and strong community support.
* JSDOM: Utilized within Jest to simulate a browser environment, enabling DOM manipulation and event handling within Node.js.
* Babel: Configured to transpile modern JavaScript (ES modules) used in the widget and tests, ensuring compatibility.
Test Coverage: The table_widget.test.js file now includes unit tests that verify:
* The basic DOM structure generated by the widget.
* The interaction logic for column header clicks, including:
* Initiating ascending sort on the first click.
* Reversing to descending sort on the second click.
* Clearing the sort (returning to unsorted state) on the third click.
* The correct display of sorting indicators (▲ for ascending, ▼ for descending, ● for unsorted).
* Proper interaction with the model.set and model.save_changes methods.
**How to Run**:
To execute these tests from the project root directory:
```bash
cd tests/js
npm install # Only needed if dependencies haven't been installed or have changed
npm test
```
This reverts commit eb2f648.
This PR introduces single-column sorting functionality to the interactive table widget.
1.1) The sort indicator dot (●) is now hidden by default and only appears when the user hovers the mouse over a column header
1.2) Implemented a sorting cycle: unsorted (●) → ascending (▲) → descending (▼) → unsorted (●).
1.3) Visual indicators (●, ▲, ▼) are displayed in column headers to reflect the current sort state.
1.4) Sorting controls are now only enabled for columns with orderable data types.
2.1) Updated
paginated_pandas_dffixture for better sorting test coverage2.2) Added new system tests to verify ascending, descending, and multi-column sorting.
3. Frontend Unit Tests
JavaScript-level unit tests have been added to validate the widget's frontend logic, specifically the new sorting functionality and UI interactions.
How to Run Frontend Unit Tests:
To execute these tests from the project root directory:
Docs has been updated to document the new features. The main description now mentions column sorting and adjustable widths, and a new section has been added to explain how to use the column resizing feature. The sorting section was also updated to mention that the indicators are only visible on hover.
Fixes #<459835971> 🦕