aminghadersohi commented on PR #40339:
URL: https://github.com/apache/superset/pull/40339#issuecomment-4585136921
Addressing all outstanding review feedback — summary by reviewer:
---
**richardfogaca (reviews 4375664950 and 4375780664 — CHANGES_REQUESTED):**
Both changes-requested reviews are now resolved:
- *`_resolve_deck_gl_metrics()` only keeps dict metrics (string-backed
`size: "count"` silently dropped):* Fixed in `362b9faaab` — added
`_is_metric_ref()` helper that accepts non-numeric strings as metric
references, so `"count"`, `"sum__sales"`, etc. are included. Numeric scalars
like `"100"` are excluded via `float()` conversion.
- *`_deck_gl_null_filters()` missing `geojson IS NOT NULL` for
`deck_geojson`:* Fixed in `362b9faaab` — the geojson field is now handled
alongside `line_column` in the null-filter loop.
- *`point_radius_fixed` bare string still ignored for `deck_scatter`:* Fixed
in `e32a683f6f` — added `elif isinstance(prf, str) and _is_metric_ref(prf)`
branch that treats non-numeric string values as metric references for legacy
scatter charts.
---
**richardfogaca (review 4382468337 — orderby and time-grain):**
Both issues are resolved in `fded66cb5d`:
- *No `orderby` on metric:* When `deck_metrics` is non-empty, the fallback
now sets `qd["orderby"] = [(deck_metrics[0], not order_desc)]`, mirroring
`BaseDeckGLViz.query_obj()`.
- *Time-grain fields dropped:* For `deck_arc`, `deck_path`, `deck_polygon`,
`deck_scatter`, and `deck_screengrid`, the fallback now propagates
`is_timeseries=True`, `granularity`, and `extras["time_grain_sqla"]` when
`time_grain_sqla` is present in form_data.
---
**aminghadersohi (review 4393314702 — M1, M2, N1, N2):**
- **M1 (MEDIUM) — `tooltip_contents` columns in `resolve_deck_gl_columns`:**
Fixed in `8285d16ecc` (just pushed). Removed the `_deck_gl_tooltip_cols()`
helper and its loop from `resolve_deck_gl_columns`. Tooltip columns are
display-only; `BaseDeckGLViz.query_obj()` does not include them in
`columns`/`groupby`. Including them would add unintended `GROUP BY` dimensions
and could fail for computed tooltip expressions. Updated tests assert that
tooltip columns are now NOT included.
- **M2 (MEDIUM) — `cross_filter_column` in `resolve_deck_gl_columns`:**
Already fixed in `5b694e901d` (prior to your review). The
`_add(form_data.get("cross_filter_column"))` line was removed and
`test_resolve_deck_gl_columns_ignores_cross_filter_column` was added as a
regression guard.
- **N1 (NIT) — No logging for legacy `point_radius_fixed` string path:**
Fixed in `8285d16ecc` — added `logger.debug("Legacy point_radius_fixed string
metric encountered: %s", prf)` so operators can identify charts that store a
bare metric string in that field.
- **N2 (NIT) — `_is_metric_ref` silent exclusion of
`"inf"`/`"-inf"`/`"nan"`:** Fixed in `8285d16ecc` — added a note in the
docstring: *"float() accepts 'inf', '-inf', and 'nan', so those strings would
be excluded here too — they are not valid metric names in practice."*
---
**Praise acknowledged** — thank you for the detailed reviews. The timeseries
branch, null filter mirroring, and `deck_geojson` special-casing were good
catches and are all covered in the test suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
