Skip to content

Conversation

@lucharo
Copy link
Contributor

@lucharo lucharo commented Sep 5, 2025

Summary

Implements multi-column sorting for marimo tables with stack-based behavior. Clicking a column sort moves it to the highest priority (end of sort array). Clicking the same direction again removes that sort. Visual indicators show sort priority (1, 2, 3...) in dropdown menus. "Clear sort" button adapts to "Clear all sorts" when multiple columns are sorted.

Backend uses list[SortArgs] where each SortArgs contains by: ColumnName and descending: bool. Frontend sends sort state as [{by: string, descending: boolean}]. Compatible with latest upstream table manager refactoring (IbisTableManager now extends NarwhalsTableManager).

🤖 Generated with Claude Code

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@vercel
Copy link

vercel bot commented Sep 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
marimo-docs Ready Ready Preview Comment Oct 6, 2025 5:24pm

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

def sort_values(
self, by: ColumnName, descending: bool
self, by: list[ColumnName], descending: list[bool]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here, list[tuple[ColumnName, bool]]

str(sort_column[i]),
# For column-oriented data (dict of lists)
data_dict = cast(dict[str, list[Any]], self.data)
indices = list(range(len(next(iter(data_dict.values())))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can simplify/optimize this, it's difficult to read.

Copy link
Contributor

@Light2Dark Light2Dark Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

            first_column = next(iter(data_dict.values()))
            num_rows = len(first_column)
            indices = list(range(num_rows))

I'm not sure if sorting for default_table is expensive. If not, then this looks fine.
If you do see optimizations, then it would be nice to add.

- Update frontend and backend to use consistent sort format: array of {by, descending} objects
- Fix TypeScript type definitions in DataTablePlugin and DataFramePlugin
- Update table manager implementations to handle list of SortArgs
- Add comprehensive multi-column sorting tests

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@github-actions github-actions bot added the bash-focus Area to focus on during release bug bash label Sep 7, 2025
@lucharo
Copy link
Contributor Author

lucharo commented Sep 7, 2025

Hey @Light2Dark, thanks for the quick review. I submitted this PR as a draft, as I still need some help with the front end that I can't quite get. I followed your comments and changed the sorting elements to tuples, which is easier to reason about as you suggested!

I also need your feedback/help on how to best present the multisort. Initially, the multi-sort was enabled by clicking the Shift key, but then I thought that'd be too implicit and doesn't really resonate with the rest of marimo's design and might even clash with some users' keyboard shortcuts. So I've settled for including a checkbox for the currently selected sorted state (descending/ascending for any given column and including an explicit "Remove sort" button), but the design is really subpar. Front-end is not my thing.

I'm hitting a union type issue I can't quite get my head around, see below for what this all looks like and the error I am getting (all in port 3000 after running make dev):

Error calling function search: Value '[{'by': 'Name', 'descending': False}]' does not fit any type of the union
Screenshot 2025-09-07 at 20 09 47

@Light2Dark
Copy link
Contributor

Light2Dark commented Sep 8, 2025

I tried out the branch, you may need to stop and run the make dev server again for it to catch the new API. I would remove the checkbox too, I don't think it's necessary. What might be nice is to have the Asc / Desc icon be clickable to remove the sort directly, instead of opening the menu. But we can do this in follow-ups.

lucharo and others added 3 commits October 3, 2025 21:36
- Implement stack-based multi-column sorting where most recent sort has highest priority
- Display priority numbers (1, 2, 3...) in sort dropdown menus
- Highlight active sort direction with bg-accent
- Toggle sort removal: clicking same direction twice removes the sort
- Adaptive clear button: "Clear sort" for single, "Clear all sorts" for multiple
- Thread table instance through column header to access sorting state

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Tests cover:
- Stack-based behavior (re-clicking moves to end)
- Toggle to remove (same direction twice)
- Adding new sorts to stack
- Direction toggling
- Priority number calculation
- Mid-stack removal

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
# Conflicts:
#	frontend/src/components/editor/ai/completion-handlers.tsx
#	marimo/_plugins/ui/_impl/tables/ibis_table.py
#	packages/openapi/api.yaml
#	tests/_plugins/ui/_impl/tables/test_ibis_table.py
Following PR feedback to avoid manual zipping/unzipping:
- Changed TableManager.sort_values() signature from list[tuple[ColumnName, bool]] to list[SortArgs]
- Updated all table manager implementations (default, narwhals)
- Removed conversion code in table.py that was creating tuples
- Updated all test files to use SortArgs objects directly

This makes the API cleaner by passing SortArgs all the way through
instead of converting to tuples and back.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
lucharo and others added 2 commits October 3, 2025 22:13
Following Light2Dark's feedback to make the code more readable:
- Extract common sort key logic into _make_sort_key() helper
- Eliminates duplicate try/except blocks for column and row-oriented paths
- More concise: 45 lines → 27 lines
- Clearer intent with inline comments explaining the approach

The helper function handles both comparable values and falls back
to string comparison when needed, putting None values last.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Frontend improvements:
- Extract isActiveSort() helper in header-items.tsx to eliminate repeated
  condition checks for highlighting and priority badges
- Extract sortArgs conversion in DataTablePlugin.tsx to avoid duplicating
  the sorting.map() transformation in multiple places

These changes make the code more maintainable by following DRY principles.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Copy link
Contributor

@Light2Dark Light2Dark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great! small review

return null;
}

// If table is available, use full multi-column sort functionality
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Lucharo, I think this will be a nicer refactor, you can use the column.toggleSorting to achieve what you want instead of manually setting the table sort

export function renderSorts<TData, TValue>(
  column: Column<TData, TValue>,
  table?: Table<TData>,
) {
  if (!column.getCanSort()) {
    return null;
  }

  const sortDirection = column.getIsSorted();
  const sortingIndex = column.getSortIndex();

  const sortingState = table?.getState().sorting;
  const hasMultiSort = sortingState?.length && sortingState.length > 1;

  const renderSortIndex = () => {
    return (
      <span className="ml-auto text-xs font-medium">{sortingIndex + 1}</span>
    );
  };

  const renderClearSort = () => {
    if (!sortDirection) {
      return null;
    }

    if (!hasMultiSort) {
      // render clear sort for this column
      return (
        <DropdownMenuItem onClick={() => column.clearSorting()}>
          <ChevronsUpDown className="mo-dropdown-icon" />
          Clear sort
        </DropdownMenuItem>
      );
    }

    // render clear sort for all columns
    return (
      <DropdownMenuItem onClick={() => table?.resetSorting()}>
        <ChevronsUpDown className="mo-dropdown-icon" />
        Clear all sorts
      </DropdownMenuItem>
    );
  };

  const toggleSort = (direction: SortDirection) => {
    // Clear sort if clicking the same direction
    if (sortDirection === direction) {
      column.clearSorting();
    } else {
      // Toggle sort direction
      const descending = direction === "desc";
      column.toggleSorting(descending, true);
    }
  };

  return (
    <>
      <DropdownMenuItem
        onClick={() => toggleSort("asc")}
        className={sortDirection === "asc" ? "bg-accent" : ""}
      >
        <AscIcon className="mo-dropdown-icon" />
        Asc
        {sortDirection === "asc" && renderSortIndex()}
      </DropdownMenuItem>
      <DropdownMenuItem
        onClick={() => toggleSort("desc")}
        className={sortDirection === "desc" ? "bg-accent" : ""}
      >
        <DescIcon className="mo-dropdown-icon" />
        Desc
        {sortDirection === "desc" && renderSortIndex()}
      </DropdownMenuItem>
      {renderClearSort()}
      <DropdownMenuSeparator />
    </>
  );
}

str(sort_column[i]),
# For column-oriented data (dict of lists)
data_dict = cast(dict[str, list[Any]], self.data)
indices = list(range(len(next(iter(data_dict.values())))))
Copy link
Contributor

@Light2Dark Light2Dark Oct 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

            first_column = next(iter(data_dict.values()))
            num_rows = len(first_column)
            indices = list(range(num_rows))

I'm not sure if sorting for default_table is expensive. If not, then this looks fine.
If you do see optimizations, then it would be nice to add.

lucharo and others added 2 commits October 4, 2025 19:22
Frontend (header-items.tsx):
- Use TanStack Table's built-in column.toggleSorting() and column.getSortIndex()
  instead of manually managing sorting state with table.setSorting()
- Cleaner code that leverages the table library's API
- Extract renderSortIndex() and renderClearSort() helper functions

Backend (default_table.py):
- Extract first_column and num_rows variables for better readability
- Makes the intent clearer when creating the indices list

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
- Add missing SortArgs import in default_table.py and narwhals_table.py
- Remove unused SortingState import in header-items.tsx

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@lucharo lucharo marked this pull request as ready for review October 4, 2025 18:47
@lucharo lucharo requested a review from manzt as a code owner October 4, 2025 18:47
@lucharo
Copy link
Contributor Author

lucharo commented Oct 4, 2025

could you re-run this step @Light2Dark , I think it was a network error: https://github.com/marimo-team/marimo/actions/runs/18248342628/job/51959424513?pr=6257

@Light2Dark
Copy link
Contributor

re-running, @lucharo there are some failing tests

lucharo and others added 2 commits October 5, 2025 14:54
- Use natural comparison for same-type values, fall back to string comparison for mixed types
- Ensure None values always appear last in sort order regardless of direction
- All sorting tests passing (25 tests)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
if sort:
# Check that all columns exist
existing_columns = set(result.get_column_names())
if all(s.by in existing_columns for s in sort):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should just filter to the columns that do exist, then sort with those. otherwise the UI may be inconsistent (it says its sorting, but it is not)

if sort:
# Check that all columns exist
existing_columns = set(result.get_column_names())
if all(s.by in existing_columns for s in sort):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, maybe just filter to valid/sortable columns

Light2Dark
Light2Dark previously approved these changes Oct 6, 2025
Copy link
Contributor

@Light2Dark Light2Dark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks for the contribution

- Filter to valid columns instead of rejecting entire sort (fixes UI inconsistency)
- Delete .claude/settings.local.json (not meant to be committed)
- Delete P_empty test artifacts

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@lucharo
Copy link
Contributor Author

lucharo commented Oct 6, 2025

CI finally passed! Thanks @Light2Dark for the review and help throughout this PR! 🙏🏼

@mscolnick mscolnick merged commit 9b58a93 into marimo-team:main Oct 6, 2025
35 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bash-focus Area to focus on during release bug bash

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants