gabotorresruiz commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5688530023
Hey Greg, I re-verified everything at `a75b6ccd` in a clean worktree with a
venv built from this branch's own requirements. Every claim below is from
execution, not reading.
All previously blocking items check out:
- Scalar `y` through the shared analyzers: driving `analyze_chart_semantics`
and `analyze_chart_capabilities` directly with a bubble config no longer
raises, and the full bubble file passes (41 tests).
- SQL-expression `x`: with `x={"sql_expression": "AVG(gdp)", "label":
"GDP"}` the story reads `This bubble_v2 chart analyzes GDP, life_expectancy`,
no `None`; the symmetric `y` case is clean too.
- Saved-chart Vega-Lite preview: `VegaLitePreviewStrategy.generate()` on a
saved bubble chart now returns a `circle` mark with `x=AVG(gdp)`,
`y=AVG(life_expectancy)`, `size=SUM(population)`, `color=continent` instead of
the generic fallback.
- Sort direction: all four callers forward `form_data.get("order_desc",
True)`, and I swept `superset/mcp_service/` for any remaining hardcoded
`order_desc=True`; there are none. I also ran your three new tests against
`edb7e9ff`: `test_saved_chart_preview_forwards_its_own_sort_direction` and
`test_fallback_forwards_the_charts_saved_direction` fail there exactly as they
should, so they genuinely pin the fix.
Since your environment could not run the tool-level files, I ran them here:
`test_get_chart_preview.py` plus `test_get_chart_data.py` 200 passed,
`test_generate_chart.py` 33 passed, and the whole
`tests/unit_tests/mcp_service/chart` tree 1635 passed, 1 skipped.
The only thing keeping me from approving is mechanical: the branch is in
conflict with master (#44004 touched `test_get_chart_data.py`; only that test
file conflicts, the source files auto-merge cleanly), and because of that
GitHub has not started the required CI on this head at all. One note for the
rebase: #44004 renamed `chart` to `chart_facts` in the saved-data fallback but
kept the hardcoded `order_desc=True` there, so your fix survives the merge and
is still needed. Once you rebase and CI comes back green I will lift my request
for changes; nothing further needed on the code itself from my side.
I am deliberately not repeating the two nits the bot raised on this head
(the duplicated ordering test and the shallow `object()` mock in the fallback
test); neither is blocking, take them or leave them.
--
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]
