Repository: marimo-team/marimo

Issue:
Table view sort always puts null values first
### Describe the bug

Sorting on a column in a table will always put nulls first, so if there are a lot of null values in the column you have to paginate through in order to see any non-null values (both in asc and desc order).

My preference is to always put nulls last, since if one is sorting on a column value, usually one wants to see rows with non-null values regardless if asc or desc.

### Environment

https://marimo.app/l/mvvscm

### Code to reproduce

```python
import marimo as mo
import pandas as pd

df = pd.DataFrame({"id": range(100), "value": list(range(50)) + [None]*50})
df
```

Hints from issue comments:
@mscolnick: Polars by default puts `nulls` first, we should dig into why as generally they've had good opinions. So does Postgres.

@mscolnick: From a duckdb discussion (and modified a bit).

> ### SQL Standard & Other Systems
> Unfortunately null ordering is extremely inconsistent across systems, and is not defined in the SQL standard if `NULLS FIRST` or `NULLS LAST` are not explicitly provided.
> 
> There are four different options for `NULL ordering`:
> 
> 1. `NULLS FIRST` when `ASC`, `NULLS LAST` when `DESC`
> 2. `NULLS LAST` when `ASC`, `NULLS FIRST` when `DESC`
> 3. Always `NULLS FIRST`
> 4. Always `NULLS LAST`
> 
> 
> ##### Other Systems
> ##### (1) `NULLS FIRST` when `ASC`, `NULLS LAST` when `DESC`
> MySQL, SQLite, BigQuery
> 
> ##### (2) `NULLS LAST` when `ASC`, `NULLS FIRST` when `DESC`
> PostgreSQL, Snowflake
> 
> ##### (3) Always `NULLS FIRST`
> DuckDB (previous), Polars, Narwhals
> 
> ##### (4) Always `NULLS LAST`
> DuckDB (new), Presto/Trino, Pandas, ClickHouse
> @akshayka: Nulls last makes sense for interactively working with data, we can do this

Solution patch:
diff --git a/frontend/src/components/editor/Cell.tsx b/frontend/src/components/editor/Cell.tsx
index 0f4e98f5..ccc39d9b 100644
--- a/frontend/src/components/editor/Cell.tsx
+++ b/frontend/src/components/editor/Cell.tsx
@@ -265,36 +265,44 @@ function useCellHiddenLogic(
   const isMarkdownCodeHidden = isMarkdown && !isCellCodeShown;
 
   // Callback to show the code editor temporarily
-  const temporarilyShowCode = useCallback(() => {
-    if (!isCellCodeShown) {
-      setTemporarilyVisible(true);
+  const temporarilyShowCode = useEvent((opts?: { focus?: boolean }) => {
+    if (isCellCodeShown) {
+      return;
+    }
+
+    // Default to true
+    const focus = opts?.focus ?? true;
+    setTemporarilyVisible(true);
+
+    if (focus) {
       editorView.current?.focus();
-      // Reach one parent up
-      const parent = editorViewParentRef.current?.parentElement;
-      if (!parent) {
-        Logger.error("Cell: No parent element found for editor view");
-        return;
-      }
+    }
 
-      const handleFocusOut = () => {
-        requestAnimationFrame(() => {
-          if (!parent.contains(document.activeElement)) {
-            // Hide the code editor
-            setTemporarilyVisible(false);
-            editorView.current?.dom.blur();
-            parent.removeEventListener("focusout", handleFocusOut);
-          }
-        });
-      };
-      parent.addEventListener("focusout", handleFocusOut);
+    // Reach one parent up
+    const parent = editorViewParentRef.current?.parentElement;
+    if (!parent) {
+      Logger.error("Cell: No parent element found for editor view");
+      return;
     }
-  }, [isCellCodeShown, editorView, editorViewParentRef, setTemporarilyVisible]);
 
-  const showHiddenMarkdownCode = useCallback(() => {
+    const handleFocusOut = () => {
+      requestAnimationFrame(() => {
+        if (!parent.contains(document.activeElement)) {
+          // Hide the code editor
+          setTemporarilyVisible(false);
+          editorView.current?.dom.blur();
+          parent.removeEventListener("focusout", handleFocusOut);
+        }
+      });
+    };
+    parent.addEventListener("focusout", handleFocusOut);
+  });
+
+  const showHiddenMarkdownCode = useEvent(() => {
     if (isMarkdownCodeHidden) {
-      temporarilyShowCode();
+      temporarilyShowCode({ focus: true });
     }
-  }, [isMarkdownCodeHidden, temporarilyShowCode]);
+  });
 
   return {
     isCellCodeShown,
diff --git a/frontend/src/components/editor/cell/code/cell-editor.tsx b/frontend/src/components/editor/cell/code/cell-editor.tsx
index cd8d2cb5..78664b93 100644
--- a/frontend/src/components/editor/cell/code/cell-editor.tsx
+++ b/frontend/src/components/editor/cell/code/cell-editor.tsx
@@ -66,7 +66,7 @@ export interface CellEditorProps
   // Props below are not used by scratchpad.
   // DOM node where the editorView will be mounted
   editorViewParentRef?: React.MutableRefObject<HTMLDivElement | null>;
-  temporarilyShowCode: () => void;
+  temporarilyShowCode: (opts?: { focus?: boolean }) => void;
 }
 
 const CellEditorInternal = ({
@@ -186,6 +186,19 @@ const CellEditorInternal = ({
           },
         };
       }),
+      // Listen to selection changes, and show the code if it is hidden
+      EditorView.updateListener.of((update) => {
+        if (update.selectionSet) {
+          const selection = update.state.selection;
+          const hasSelection = selection.ranges.some(
+            (range) => range.from !== range.to,
+          );
+
+          if (hasSelection) {
+            temporarilyShowCode({ focus: false });
+          }
+        }
+      }),
     );
 
     return extensions;
@@ -206,6 +219,7 @@ const CellEditorInternal = ({
     setAiCompletionCell,
     afterToggleMarkdown,
     setLanguageAdapter,
+    temporarilyShowCode,
   ]);
 
   const handleInitializeEditor = useEvent(() => {
diff --git a/marimo/_plugins/ui/_impl/tables/default_table.py b/marimo/_plugins/ui/_impl/tables/default_table.py
index 8586fea1..c5451ced 100644
--- a/marimo/_plugins/ui/_impl/tables/default_table.py
+++ b/marimo/_plugins/ui/_impl/tables/default_table.py
@@ -2,12 +2,7 @@
 from __future__ import annotations
 
 from collections.abc import Sequence
-from typing import (
-    Any,
-    Optional,
-    Union,
-    cast,
-)
+from typing import Any, Optional, Union, cast
 
 from marimo._data.models import ColumnSummary, ExternalDataType
 from marimo._dependencies.dependencies import DependencyManager
@@ -41,6 +36,10 @@ JsonTableData = Union[
     dict[str, JSONType],
 ]
 
+# For non-column-oriented data, we use "key" and "value" as the column names
+KEY = "key"
+VALUE = "value"
+
 
 class DefaultTableManager(TableManager[JsonTableData]):
     type = "dictionary"
@@ -297,6 +296,8 @@ class DefaultTableManager(TableManager[JsonTableData]):
 
     def get_column_names(self) -> list[str]:
         if isinstance(self.data, dict):
+            if not self.is_column_oriented:
+                return [KEY, VALUE]
             return list(self.data.keys())
         first = next(iter(self.data), None)
         return list(first.keys()) if isinstance(first, dict) else ["value"]
@@ -323,9 +324,27 @@ class DefaultTableManager(TableManager[JsonTableData]):
                 )
             except TypeError:
                 # Handle when values are not comparable
+                def sort_func_str(i: int) -> tuple[bool, str] | str:
+                    # For ascending, generate a tuple of (is_none, value)
+                    # (True, None) will be for None values
+                    # (False, x) will be for other values.
+                    # As False < True, None values will be sorted to the end.
+
+                    # For descending, (is_not_none, value) tuple
+                    # (False, None) will be for None values.
+                    # (True, x) will be for other values.
+                    # As True > False, other values come before None values
+                    if descending:
+                        return (
+                            sort_column[i] is not None,
+                            str(sort_column[i]),
+                        )
+                    else:
+                        return str(sort_column[i])
+
                 sorted_indices = sorted(
                     range(len(sort_column)),
-                    key=lambda i: str(sort_column[i]),
+                    key=sort_func_str,
                     reverse=descending,
                 )
             # Apply sorted indices to each column while maintaining column orientation
@@ -344,11 +363,20 @@ class DefaultTableManager(TableManager[JsonTableData]):
         # For row-major data, continue with existing logic
         normalized = self._normalize_data(self.data)
         try:
-            data = sorted(normalized, key=lambda x: x[by], reverse=descending)
+
+            def sort_func_col(x: dict[str, Any]) -> tuple[bool, Any]:
+                is_none = x[by] is not None if descending else x[by] is None
+                return (is_none, x[by])
+
+            data = sorted(normalized, key=sort_func_col, reverse=descending)
         except TypeError:
             # Handle when all values are not comparable
+            def sort_func_col_str(x: dict[str, Any]) -> tuple[bool, str]:
+                is_none = x[by] is not None if descending else x[by] is None
+                return (is_none, str(x[by]))
+
             data = sorted(
-                normalized, key=lambda x: str(x[by]), reverse=descending
+                normalized, key=sort_func_col_str, reverse=descending
             )
         return DefaultTableManager(data)
 
@@ -374,9 +402,7 @@ class DefaultTableManager(TableManager[JsonTableData]):
 
         # If its a dictionary, convert to key-value pairs
         if isinstance(data, dict):
-            return [
-                {"key": key, "value": value} for key, value in data.items()
-            ]
+            return [{KEY: key, VALUE: value} for key, value in data.items()]
 
         # Assert that data is a list
         if not isinstance(data, (list, tuple)):
diff --git a/marimo/_plugins/ui/_impl/tables/narwhals_table.py b/marimo/_plugins/ui/_impl/tables/narwhals_table.py
index 1b2869d0..f79ad761 100644
--- a/marimo/_plugins/ui/_impl/tables/narwhals_table.py
+++ b/marimo/_plugins/ui/_impl/tables/narwhals_table.py
@@ -395,11 +395,11 @@ class NarwhalsTableManager(
     ) -> TableManager[Any]:
         if isinstance(self.data, nw.LazyFrame):
             return self.with_new_data(
-                self.data.sort(by, descending=descending)
+                self.data.sort(by, descending=descending, nulls_last=True)
             )
         else:
             return self.with_new_data(
-                self.data.sort(by, descending=descending)
+                self.data.sort(by, descending=descending, nulls_last=True)
             )
 
     def __repr__(self) -> str:


Test patch:
diff --git a/tests/_plugins/ui/_impl/tables/test_default_table.py b/tests/_plugins/ui/_impl/tables/test_default_table.py
index a657cf58..dbcac368 100644
--- a/tests/_plugins/ui/_impl/tables/test_default_table.py
+++ b/tests/_plugins/ui/_impl/tables/test_default_table.py
@@ -125,6 +125,46 @@ class TestDefaultTable(unittest.TestCase):
         ]
         assert sorted_data == expected_data
 
+    def test_sort_null_values(self) -> None:
+        data_with_nan = self.data.copy()
+        data_with_nan[1]["age"] = None
+        manager_with_nan = DefaultTableManager(data_with_nan)
+        sorted_data = manager_with_nan.sort_values(
+            by="age", descending=False
+        ).data
+        last_row = sorted_data[-1]
+
+        expected_last_row = {
+            "name": "Bob",
+            "age": None,
+            "birth_year": date(1999, 7, 14),
+        }
+
+        # ascending
+        assert last_row == expected_last_row
+
+        # descending
+        sorted_data = manager_with_nan.sort_values(
+            by="age", descending=True
+        ).data
+        last_row = sorted_data[-1]
+        assert last_row == expected_last_row
+
+        # strings ascending
+        data_with_strings = self.data.copy()
+        data_with_strings[1]["name"] = None
+        manager_with_strings = DefaultTableManager(data_with_strings)
+        sorted_data = manager_with_strings.sort_values(
+            by="name", descending=False
+        ).data
+        assert sorted_data[-1]["name"] is None
+
+        # strings descending
+        sorted_data = manager_with_strings.sort_values(
+            by="name", descending=True
+        ).data
+        assert sorted_data[-1]["name"] is None
+
     def test_sort_single_values(self) -> None:
         manager = DefaultTableManager([1, 3, 2])
         sorted_data = manager.sort_values(by="value", descending=True).data
@@ -419,6 +459,39 @@ class TestColumnarDefaultTable(unittest.TestCase):
         }
         assert sorted_data == expected_data
 
+    def test_sort_null_values(self) -> None:
+        data_with_nan = self.data.copy()
+        data_with_nan["age"][1] = None
+        manager_with_nan = DefaultTableManager(data_with_nan)
+        sorted_data = manager_with_nan.sort_values(
+            by="age", descending=False
+        ).data
+
+        assert sorted_data["age"][-1] is None
+        assert sorted_data["name"][-1] == "Bob"
+
+        # ascending
+        sorted_data = manager_with_nan.sort_values(
+            by="age", descending=True
+        ).data
+        assert sorted_data["age"][-1] is None
+        assert sorted_data["name"][-1] == "Bob"
+
+        # strings ascending
+        data_with_strings = self.data.copy()
+        data_with_strings["name"][1] = None
+        manager_with_strings = DefaultTableManager(data_with_strings)
+        sorted_data = manager_with_strings.sort_values(
+            by="name", descending=False
+        ).data
+        assert sorted_data["name"][-1] is None
+
+        # strings descending
+        sorted_data = manager_with_strings.sort_values(
+            by="name", descending=True
+        ).data
+        assert sorted_data["name"][-1] is None
+
     @pytest.mark.skipif(
         not HAS_DEPS, reason="optional dependencies not installed"
     )
@@ -650,6 +723,9 @@ class TestDictionaryDefaultTable(unittest.TestCase):
             TableCell(row=1, column="value", value=2),
         ]
 
+    @pytest.mark.xfail(
+        reason="get_column_names() doesn't work properly for row-oriented dicts"
+    )
     def test_drop_columns(self) -> None:
         dropped_manager = self.manager.drop_columns(["a"])
         assert dropped_manager.data == {"b": 2}
@@ -679,6 +755,50 @@ class TestDictionaryDefaultTable(unittest.TestCase):
         expected_data = [{"key": "b", "value": 2}, {"key": "a", "value": 1}]
         assert sorted_manager.data == expected_data
 
+    def test_sort_null_values(self) -> None:
+        data = self.manager.data.copy()
+        data["b"] = None
+        manager_with_nan = DefaultTableManager(data)
+        sorted_data = manager_with_nan.sort_values(
+            by="value", descending=False
+        ).data
+        assert sorted_data == [
+            {"key": "a", "value": 1},
+            {"key": "b", "value": None},
+        ]
+
+        # descending
+        sorted_data = manager_with_nan.sort_values(
+            by="value", descending=True
+        ).data
+        assert sorted_data == [
+            {"key": "a", "value": 1},
+            {"key": "b", "value": None},
+        ]
+
+        # strings ascending
+        data_with_strings = DefaultTableManager(
+            {"a": "foo", "b": None, "c": "bar"}
+        )
+        sorted_data = data_with_strings.sort_values(
+            by="value", descending=False
+        ).data
+        assert sorted_data == [
+            {"key": "c", "value": "bar"},
+            {"key": "a", "value": "foo"},
+            {"key": "b", "value": None},
+        ]
+
+        # strings descending
+        sorted_data = data_with_strings.sort_values(
+            by="value", descending=True
+        ).data
+        assert sorted_data == [
+            {"key": "a", "value": "foo"},
+            {"key": "c", "value": "bar"},
+            {"key": "b", "value": None},
+        ]
+
     def test_search(self) -> None:
         searched_manager = self.manager.search("a")
         assert searched_manager.data == [{"key": "a", "value": 1}]
diff --git a/tests/_plugins/ui/_impl/tables/test_narwhals.py b/tests/_plugins/ui/_impl/tables/test_narwhals.py
index 0f5b9d6d..4c91bb22 100644
--- a/tests/_plugins/ui/_impl/tables/test_narwhals.py
+++ b/tests/_plugins/ui/_impl/tables/test_narwhals.py
@@ -834,9 +834,15 @@ def test_search_with_regex(df: Any) -> None:
 def test_sort_values_with_nulls(df: Any) -> None:
     manager = NarwhalsTableManager.from_dataframe(df)
     sorted_manager = manager.sort_values("A", descending=True)
-    first = unwrap_py_scalar(sorted_manager.data["A"][0])
-    assert first is None or isnan(first)
-    assert sorted_manager.data["A"].to_list()[1:] == [3, 2, 1]
+    assert sorted_manager.data["A"].to_list()[:-1] == [3, 2, 1]
+    last = unwrap_py_scalar(sorted_manager.data["A"][-1])
+    assert last is None or isnan(last)
+
+    # ascending
+    sorted_manager = manager.sort_values("A", descending=False)
+    assert sorted_manager.data["A"].to_list()[:-1] == [1, 2, 3]
+    last = unwrap_py_scalar(sorted_manager.data["A"][-1])
+    assert last is None or isnan(last)
 
 
 @pytest.mark.skipif(not HAS_DEPS, reason="optional dependencies not installed")
diff --git a/tests/_plugins/ui/_impl/tables/test_pandas_table.py b/tests/_plugins/ui/_impl/tables/test_pandas_table.py
index d9d186fb..5420d414 100644
--- a/tests/_plugins/ui/_impl/tables/test_pandas_table.py
+++ b/tests/_plugins/ui/_impl/tables/test_pandas_table.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 import datetime
 import json
 import unittest
+from math import isnan
 from typing import Any
 from unittest.mock import Mock
 
@@ -962,11 +963,23 @@ class TestPandasTableManager(unittest.TestCase):
         df = pd.DataFrame({"A": [3, 1, None, 2]})
         manager = self.factory.create()(df)
         sorted_manager = manager.sort_values("A", descending=True)
-        assert sorted_manager.data["A"].to_list()[1:] == [
+        assert sorted_manager.data["A"].to_list()[:-1] == [
             3.0,
             2.0,
             1.0,
         ]
+        last = sorted_manager.data["A"][-1]
+        assert last is None or isnan(last)
+
+        # ascending
+        sorted_manager = manager.sort_values("A", descending=False)
+        assert sorted_manager.data["A"].to_list()[:-1] == [
+            1.0,
+            2.0,
+            3.0,
+        ]
+        last = sorted_manager.data["A"][-1]
+        assert last is None or isnan(last)
 
     def test_dataframe_with_multiindex(self) -> None:
         df = pd.DataFrame(
diff --git a/tests/_plugins/ui/_impl/tables/test_polars_table.py b/tests/_plugins/ui/_impl/tables/test_polars_table.py
index c1c98753..db994477 100644
--- a/tests/_plugins/ui/_impl/tables/test_polars_table.py
+++ b/tests/_plugins/ui/_impl/tables/test_polars_table.py
@@ -3,6 +3,7 @@ from __future__ import annotations
 import datetime
 import json
 import unittest
+from math import isnan
 from typing import Any
 
 import narwhals.stable.v1 as nw
@@ -814,7 +815,23 @@ class TestPolarsTableManagerFactory(unittest.TestCase):
         df = pl.DataFrame({"A": [3, 1, None, 2]})
         manager = self.factory.create()(df)
         sorted_manager = manager.sort_values("A", descending=True)
-        assert sorted_manager.data["A"].to_list() == [None, 3, 2, 1]
+        assert sorted_manager.data["A"].to_list()[:-1] == [
+            3.0,
+            2.0,
+            1.0,
+        ]
+        last = sorted_manager.data["A"][-1]
+        assert last is None or isnan(last)
+
+        # ascending
+        sorted_manager = manager.sort_values("A", descending=False)
+        assert sorted_manager.data["A"].to_list()[:-1] == [
+            1.0,
+            2.0,
+            3.0,
+        ]
+        last = sorted_manager.data["A"][-1]
+        assert last is None or isnan(last)
 
     def test_get_field_types_with_datetime(self):
         import polars as pl