gkneighb commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5658379340
New head `edb7e9ff01` picks up three of the four suggestions from the last
automated pass. Two of them were real defects in code I added in `c5439587`, so
thanks to whoever tuned that bot — flagging them as non-blocking undersold them.
**Sort direction ignored the caller's flag** (`chart_helpers.py`). My
`orderby` branch read `form_data.get("order_desc", True)` while
`_build_single_query_dict` has its own `order_desc` parameter that sets
`qd["order_desc"]` a few lines above. `get_chart_data` passes it explicitly, so
a saved `order_desc=False` plus an explicit `order_desc=True` produced a query
dict that sorted ascending while declaring itself descending. The argument now
wins, with form_data as the fallback:
```
build_query_dicts_from_form_data(fd_with_order_desc_false, ...,
order_desc=True)
-> order_desc True, orderby [(SUM(population), False)]
```
**Unlabeled metrics were dropped from the bubble preview**
(`preview_utils.py`). I wrote a local `_metric_field_name` that only understood
an explicit `label`, when `metric_result_label` was already imported in the
same module (line 31) and handles the cases mine didn't — reconstructing
`AVG(gdp)` from a SIMPLE metric's aggregate plus column, and resolving
SQL-expression metrics. For a saved chart whose form_data carries unlabeled
metrics, my version returned `None` and the channel disappeared from the spec
entirely; the new test fails with `KeyError: 'x'` against the old code. The
duplicate is gone and the shared resolver is used.
**Test strengthened.** `test_saved_bubble_with_sql_expression_x_is_reported`
asserted only that the chart came back, so it would have passed with `"None"`
sitting in the data story — the exact thing the previous commit fixed. It now
asserts the label is present and `"None"` is absent.
The fourth suggestion — that the `sql_expression`/`saved_metric` guard on
`entity`/`series` in `normalize_column_refs` is unreachable because the config
validator already rejects both — is correct. I've left it as defensive code
rather than grow this diff further; happy to drop it if a reviewer would rather
see it go.
For the record on the previous head: the required check was red from the
30-minute job limit again, not a test failure. The JUnit artifact for that run
recorded `tests=15174 failures=0 errors=0`, with all three new tests passing.
--
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]
