Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions hotdata_marimo/_options.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
"""Shared dropdown option helpers for Marimo UI widgets."""

from __future__ import annotations

from collections.abc import Callable
from typing import Any

import marimo as mo

from hotdata_runtime import HotdataClient


def unique_label_options(
pairs: list[tuple[str, str]],
*,
disambiguate: Callable[[str, str, int], str] | None = None,
) -> dict[str, str]:
"""Build a label→value map, suffixing repeated labels when needed."""
counts: dict[str, int] = {}
options: dict[str, str] = {}
for label, value in pairs:
count = counts.get(label, 0)
counts[label] = count + 1
if count == 0:
key = label
elif disambiguate is not None:
key = disambiguate(label, value, count)
else:
key = f"{label} ({count + 1})"
options[key] = value
return options


def empty_dropdown(
*,
label: str,
message: str,
full_width: bool = True,
):
return mo.ui.dropdown(
options={message: ""},
label=label,
full_width=full_width,
)


def connection_options(conns: list[Any]) -> dict[str, str]:
pairs = [(str(c.name), str(c.id)) for c in conns]
return unique_label_options(
pairs,
disambiguate=lambda label, value, count: f"{label} ({value})",
)


def connection_picker(
client: HotdataClient,
*,
label: str = "Connection",
full_width: bool = True,
):
listing = client.connections().list_connections()
conns = listing.connections
if not conns:
return empty_dropdown(
label=label,
message="(no connections)",
full_width=full_width,
)
return mo.ui.dropdown(
options=connection_options(conns),
label=label,
full_width=full_width,
)


def resolve_connection_picker(
client: HotdataClient,
*,
label: str = "Connection",
full_width: bool = True,
) -> tuple[Any | None, str | None]:
"""Return ``(dropdown_or_none, implicit_connection_id)`` for table browsers."""
listing = client.connections().list_connections()
conns = listing.connections
if not conns:
return None, ""
if len(conns) == 1:
return None, conns[0].id
return connection_picker(client, label=label, full_width=full_width), None
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: when len(conns) > 1, resolve_connection_picker calls client.connections().list_connections() here, then immediately calls connection_picker(client, ...) which fetches the same listing again. Since you've just fetched conns, consider passing them through to avoid the duplicate API round-trip (e.g., split connection_picker so the dropdown can be built from a pre-fetched conns list). The pre-refactor code had the same duplicate fetch, so this isn't a regression — just an opportunity now that the logic is in one place. (not blocking)

27 changes: 10 additions & 17 deletions hotdata_marimo/display.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,7 @@

from hotdata_runtime import HotdataClient, QueryResult, workspace_health_lines


def _option_map_with_unique_labels(
pairs: list[tuple[str, str]],
) -> dict[str, str]:
counts: dict[str, int] = {}
options: dict[str, str] = {}
for label, value in pairs:
count = counts.get(label, 0)
counts[label] = count + 1
key = label if count == 0 else f"{label} ({count + 1})"
options[key] = value
return options
from hotdata_marimo._options import empty_dropdown, unique_label_options


def query_result(
Expand Down Expand Up @@ -76,11 +65,15 @@ def __init__(self, client: HotdataClient, *, limit: int = 50) -> None:
(f"{r.created_at} · {r.status} · {r.result_id}", r.result_id)
for r in self._results
]
options = _option_map_with_unique_labels(option_pairs)
self.pick = mo.ui.dropdown(
options=options or {"(no results)": ""},
label="Recent results",
full_width=True,
options = unique_label_options(option_pairs)
self.pick = (
empty_dropdown(label="Recent results", message="(no results)")
if not options
else mo.ui.dropdown(
options=options,
label="Recent results",
full_width=True,
)
)

@property
Expand Down
14 changes: 5 additions & 9 deletions hotdata_marimo/sql_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ def __init__(
) -> None:
super().__init__(connection, engine_name)
self._connections_cache: list[Any] | None = None
self._connection_id_cache: dict[str, str] | None = None

@property
def source(self) -> str:
Expand Down Expand Up @@ -71,15 +70,12 @@ def _resolve_should_auto_discover(
return True
return value

def _connection_ids(self) -> dict[str, str]:
if self._connection_id_cache is None:
self._connection_id_cache = {
str(c.name): str(c.id) for c in self._connections()
}
return self._connection_id_cache

def _connection_id(self, connection_name: str) -> str | None:
return self._connection_ids().get(connection_name)
try:
return self._connection.connection_id_by_name().get(connection_name)
except RuntimeError as e:
LOGGER.warning("%s", e)
return None
Comment on lines 73 to +78
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: this drops the local _connection_id_cache and now calls self._connection.connection_id_by_name() on every invocation. _connection_id is hit from get_schemas, get_tables_in_schema, and get_table_details — each of which can fire several times per render — so unless connection_id_by_name() is cheap or memoized on the runtime side, this will multiply backend calls during catalog browsing. Worth confirming the runtime caches the mapping, or memoize locally (e.g., one cached dict reused alongside _connections_cache). (not blocking)


def _connections(self) -> list[Any]:
if self._connections_cache is None:
Expand Down
99 changes: 33 additions & 66 deletions hotdata_marimo/table_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,39 +6,11 @@

from hotdata_runtime import HotdataClient


def _connection_options(conns: list[Any]) -> dict[str, str]:
counts: dict[str, int] = {}
options: dict[str, str] = {}
for c in conns:
label = c.name
count = counts.get(label, 0)
counts[label] = count + 1
key = label if count == 0 else f"{label} ({c.id})"
options[key] = c.id
return options


def connection_picker(
client: HotdataClient,
*,
label: str = "Connection",
full_width: bool = True,
):
listing = client.connections().list_connections()
conns = listing.connections
if not conns:
return mo.ui.dropdown(
options={"(no connections)": ""},
label=label,
full_width=full_width,
)
options = _connection_options(conns)
return mo.ui.dropdown(
options=options,
label=label,
full_width=full_width,
)
from hotdata_marimo._options import (
connection_picker,
empty_dropdown,
resolve_connection_picker,
)
Comment on lines +9 to +13
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

super nit: connection_picker isn't referenced anywhere in this module's body — it's imported solely so that from hotdata_marimo.table_browser import connection_picker in __init__.py keeps working. That's fine, but a one-line comment (or # re-exported for backwards-compat) would make the intent obvious to a future reader who might otherwise drop it as an unused import. (not blocking)



class TableBrowser:
Expand Down Expand Up @@ -66,44 +38,40 @@ def __init__(
self._implicit_connection_id: str | None = None

if self._override_connection_id is None:
listing = client.connections().list_connections()
conns = listing.connections
if len(conns) > 1:
self._conn_pick = connection_picker(client)
elif len(conns) == 1:
self._implicit_connection_id = conns[0].id
else:
self._implicit_connection_id = ""
self._conn_pick, self._implicit_connection_id = resolve_connection_picker(
client
)

self._table_pick_ctx: str | None = None
self._init_table_pick()

def _init_table_pick(self) -> None:
if self._conn_pick is not None:
self.table_pick = mo.ui.dropdown(
options={"(select connection above)": ""},
self.table_pick = empty_dropdown(
label="Table",
full_width=True,
message="(select connection above)",
)
self._empty_catalog = True
self._all_names = []
return

names = self._names_for_active_connection()
self._all_names = names
if not names:
self.table_pick = empty_dropdown(
label="Table",
message="(no tables in catalog)",
)
self._empty_catalog = True
self._all_names: list[str] = []
else:
names = self._names_for_active_connection()
self._all_names = names
if not names:
self.table_pick = mo.ui.dropdown(
options={"(no tables in catalog)": ""},
label="Table",
full_width=True,
)
self._empty_catalog = True
else:
self._empty_catalog = False
self.table_pick = mo.ui.dropdown(
options={n: n for n in names},
label="Table",
full_width=True,
searchable=True,
)
self._table_pick_ctx = self._active_connection_id()
self._empty_catalog = False
self.table_pick = mo.ui.dropdown(
options={n: n for n in names},
label="Table",
full_width=True,
searchable=True,
)
self._table_pick_ctx = self._active_connection_id()

def _active_connection_id(self) -> str | None:
if self._override_connection_id is not None:
Expand All @@ -128,10 +96,9 @@ def _rebuild_table_pick(self, names: list[str]) -> None:
self._all_names = names
if not names:
self._empty_catalog = True
self.table_pick = mo.ui.dropdown(
options={"(no tables in catalog)": ""},
self.table_pick = empty_dropdown(
label="Table",
full_width=True,
message="(no tables in catalog)",
)
else:
self._empty_catalog = False
Expand Down
27 changes: 16 additions & 11 deletions hotdata_marimo/workspace_selector.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
resolve_workspace_selection,
)

from hotdata_marimo._options import unique_label_options


class WorkspaceSelector:
"""Workspace picker that rebuilds `HotdataClient` as selection changes."""
Expand Down Expand Up @@ -37,17 +39,20 @@ def __init__(
self._workspace_id = workspaces[0].public_id
return

labels: list[tuple[str, str]] = []
seen: set[str] = set()
for w in workspaces:
base = w.name
label_text = base if base not in seen else f"{base} ({w.public_id})"
seen.add(base)
labels.append((label_text, w.public_id))

labels.sort(key=lambda t: 0 if t[1] == selection.workspace_id else 1)
options = {k: v for k, v in labels}
self._pick = mo.ui.dropdown(options=options, label=label, full_width=True)
pairs = [(w.name, w.public_id) for w in workspaces]
options = unique_label_options(
pairs,
disambiguate=lambda name, public_id, count: f"{name} ({public_id})",
)
items = sorted(
options.items(),
key=lambda item: 0 if item[1] == selection.workspace_id else 1,
)
self._pick = mo.ui.dropdown(
options=dict(items),
label=label,
full_width=True,
)
self._workspace_id = selection.workspace_id

@property
Expand Down
9 changes: 4 additions & 5 deletions tests/test_hotdata_marimo.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@

import hotdata_marimo as hm
from hotdata_runtime import HotdataClient
from hotdata_marimo.display import _option_map_with_unique_labels
from hotdata_marimo._options import connection_options, unique_label_options
from hotdata_marimo.sql_engine import HotdataMarimoEngine
from hotdata_marimo.table_browser import _connection_options
from hotdata_marimo.workspace_selector import WorkspaceSelector, workspace_selector_from_env
from marimo._types.ids import VariableName

Expand Down Expand Up @@ -39,8 +38,8 @@ def _workspace_row(name: str, public_id: str, *, active: bool = True):
),
],
)
def test_option_map_with_unique_labels(labels, expected):
assert _option_map_with_unique_labels(labels) == expected
def test_unique_label_options(labels, expected):
assert unique_label_options(labels) == expected


def test_connection_options_disambiguates_duplicate_names():
Expand All @@ -49,7 +48,7 @@ def test_connection_options_disambiguates_duplicate_names():
SimpleNamespace(name="Warehouse", id="conn_2"),
SimpleNamespace(name="Analytics", id="conn_3"),
]
assert _connection_options(conns) == {
assert connection_options(conns) == {
"Warehouse": "conn_1",
"Warehouse (conn_2)": "conn_2",
"Analytics": "conn_3",
Expand Down
Loading