Add headless interactive charting#188
Conversation
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
1ef2fca to
cb4bfed
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4bfed4cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def to_metadata_spec(self, *, query_endpoint: str | None = None) -> dict[str, Any]: | ||
| if self._spec is None: | ||
| self._spec = self.chart.to_crossfilter_metadata() |
There was a problem hiding this comment.
When a non-initial tab is fetched through the lazy /crossfilter/spec route, this stores the empty data_deferred metadata spec in the same _spec cache used by spec, to_spec(), and source_record_count. After that request, any dashboard without an explicit source_record_count reports total_source_rows as 0 from the empty data, and later full spec/export calls for the same session also remain empty instead of materializing rows. Use a separate metadata cache or return the metadata spec without assigning _spec.
Useful? React with 👍 / 👎.
| metric_selects = [ | ||
| f"{_aggregate_metric_sql(metric, self.metric_aggs.get(metric))} AS {_identifier(metric)}" | ||
| for metric in metrics |
There was a problem hiding this comment.
When interaction preaggregations are disabled or unsupported, the crossfilter trend/KPI/bar fallback first queries metrics grouped by all chart dimensions and then rolls those grouped rows up here. For an avg metric, _aggregate_metric_sql produces AVG(metric), which averages the per-group averages rather than recomputing from the underlying rows, so brushing a small group and a large group weights them equally and returns incorrect dashboard values. This affects any crossfilter chart using agg="avg" metrics in the fallback path.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
cb4bfed to
160fea4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160fea47bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"{base_ref} >= DATE {_literal(str(min_value)[:10])}", | ||
| f"{base_ref} < DATE {_literal(upper.isoformat())}", | ||
| ] | ||
| return [f"{field_ref} >= {_literal(min_value)}", f"{field_ref} <= {_literal(max_value)}"] |
There was a problem hiding this comment.
When the x-axis uses a time grain other than __day or __month (for example orders.created_at__year or __week), live crossfilter requests send a TimeRange that falls through to this branch and compiles filters like orders.created_at__year >= '2025-01-01'. The normal SQL generator pushes single-model dimension filters into the model CTE, where only raw columns/selected aliases exist, so these suffixed semantic references are not valid table columns and brushing those charts returns a SQL error instead of filtered views. The day/month branches avoid this by filtering on the base time ref; the other supported grains need equivalent base-column bounds or date truncation.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
160fea4 to
84c1a2d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84c1a2d399
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| values => ({ | ||
| value: d3.sum(values, row => Number(row[yField])), |
There was a problem hiding this comment.
When a crossfilter chart is exported as static HTML/JSON without a live endpoint (for example sidemantic chart ... --renderer crossfilter --format html), liveViews is null and the browser rolls already-aggregated semantic rows up with d3.sum. This is only correct for additive metrics; avg, min, max, or count_distinct dashboards with extra dimensions will show incorrect trend/breakdown/KPI values because the raw rows needed to recompute those aggregates are no longer available. The fresh path here is the browser-only static renderer, not the server fallback discussed earlier.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
84c1a2d to
99c3d3b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99c3d3b37b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _source_record_count_from_spec(spec: dict[str, Any]) -> int: | ||
| count_field = next((field for field in spec["fields"]["metrics"] if "count" in field.lower()), None) | ||
| if count_field is None: | ||
| return len(spec["data"]) | ||
| return int(sum(row.get(count_field, 0) for row in spec["data"])) |
There was a problem hiding this comment.
When a crossfilter spec omits source_record_count, this picks the first metric whose alias contains the substring count, so ordinary metrics like discount_amount or account_balance are treated as row counts and summed into total_source_rows. That makes the UI and live response report nonsensical source-record totals for those dashboards; use the metric_aggs metadata to select actual agg == "count" metrics instead of matching the alias text.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
99c3d3b to
b14f9c1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b14f9c1869
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output_path = Path(output_dir) | ||
| if not self.query_endpoint: | ||
| raise ValueError("CrossfilterDashboard.serve requires a query_endpoint") | ||
| self.write(output_path) |
There was a problem hiding this comment.
In the CLI dashboard serve path this calls write(), which serializes to_spec() and therefore materializes every tab's full chart data before the HTTP server starts. The ASGI path avoids this with to_lazy_html()//crossfilter/spec, but CLI usage of multi-tab dashboards with large non-initial tabs still blocks startup and runs all tab queries eagerly; write a lazy shell or add a GET spec route for this server path instead.
Useful? React with 👍 / 👎.
| "kpis": kpis["sql"], | ||
| **{f"bar:{field}": sql for field, sql in bar_sqls.items()}, | ||
| } | ||
| total_groups = len(self.spec.get("data") or []) or len(current["rows"]) |
There was a problem hiding this comment.
When a tab is loaded from the metadata-only lazy spec and a source_record_count was supplied, self.spec still has no data, so filtered requests fall back to len(current["rows"]) here. In that scenario current["rows"] is already filtered, causing live responses and the browser summary to report totals like 3 of 3 groups instead of the tab's unfiltered group count; keep the unfiltered group count from the initial query or compute it separately.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
b14f9c1 to
57b1a10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57b1a10b9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| encoding = chart.get("encoding") or {} | ||
| dimensions = _ordered_dimensions(_as_list(query.get("dimensions")), encoding) | ||
| builder = layer.chart( | ||
| _as_list(query.get("metrics")), |
There was a problem hiding this comment.
When a dashboard chart declares multiple metrics and sets encoding.y to a metric that is not first in query.metrics, this path passes the original metric order into layer.chart unchanged. The bundled crossfilter runtime then assigns yField = metrics[0] in sidemantic/viz.py, so the chart renders and filters the first query metric instead of the metric the dashboard spec explicitly selected; this affects common configs that include supporting KPI/scatter metrics before the desired primary y metric.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
57b1a10 to
8df7ecd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8df7ecd531
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dimensions=query_dimensions or None, | ||
| filters=_as_list(query.get("filters")) or None, | ||
| segments=_as_list(query.get("segments")) or None, | ||
| order_by=_order_by(query) or None, |
There was a problem hiding this comment.
When a dashboard spec has a typo in query.orderBy/order_by (for example orders.revene), validation passes because these values are never checked against fields and layer.compile() only serializes the ORDER BY string without executing it. The next dashboard serve request then emits SQL ordered by a missing column and fails at query time, so sidemantic dashboard validate can report a broken dashboard as valid.
Useful? React with 👍 / 👎.
| }); | ||
| if (!response.ok) throw new Error(`Crossfilter query failed: ${response.status}`); | ||
| const payload = await response.json(); | ||
| if (requestId !== pendingRequestId) return; |
There was a problem hiding this comment.
When a live request is in flight and the user switches tabs before it returns, this check only compares the request id, not the tab the request was sent for. If the destination tab already has a spec (for example switching back to the initial tab) or its refresh has not yet incremented pendingRequestId, the old tab's rows are installed into the newly active tab and rendered with the wrong fields; capture the requested tab id and discard responses that no longer match currentTabId().
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
8df7ecd to
c56cd83
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c56cd83840
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type: 'xRange', | ||
| field: xField, |
There was a problem hiding this comment.
When a crossfilter chart is built without any dimensions (for example the CLI's allowed sidemantic chart orders.revenue --renderer crossfilter --serve), xField is the first metric, but brushing still sends an xRange filter here. The server coerces every xRange into a TimeRange and resolves it via dimension_ref, so the next live request fails with an unknown crossfilter dimension instead of filtering the metric-only chart; either disable/translate this brush for metric x-fields or reject dimensionless crossfilter specs.
Useful? React with 👍 / 👎.
| return CrossfilterQueryResponse( | ||
| rows=current["rows"], | ||
| total_groups=total_groups, | ||
| total_source_rows=self.session.source_record_count, |
There was a problem hiding this comment.
For a lazy-loaded tab without an explicit source_record_count, the first live POST still reaches this property with _source_record_count and _spec unset, so source_record_count calls self.spec and executes chart.to_crossfilter() to materialize the full grouped dataset after the /crossfilter/spec metadata route intentionally deferred it. That makes large non-initial tabs do the full eager query in addition to the live view queries; compute the count from the live/current query when unfiltered or use a separate count path instead of forcing the full spec.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
interactive-charting-crossfilter
branch
from
c56cd83 to
8b3d8b5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Adds a headless semantic charting layer and live crossfilter dashboard runtime.
What changed:
Notes: