aminghadersohi commented on PR #44603:
URL: https://github.com/apache/superset/pull/44603#issuecomment-5942569519
@rebenitez1802 — thanks, every item is addressed in `f870d06d3f` (branch
head `f1db7e7142`, master merged in). One line each, in your order:
1. **🟡 Saved-metric near-misses / `available_metrics: []`** — fixed in
`f870d06d3f`. `available_metrics` is populated with sanitized names (bounded to
10), and a ref carrying an `aggregate` whose name near-misses a metric now gets
a `{"name": "sum_boys", "saved_metric": true}` hint directly, so it lands in
one step rather than two via `saved_metric_not_marked`. A physical-column match
still wins where one exists, and a dimension slot never gets the hint. Metrics
stay excluded from dimension/`WHERE` candidates as you asked.
2. **Multi-column candidates / first column takes all 3 slots** — fixed in
`f870d06d3f`. `_rank_candidates` fills the shared cap round-robin via
`zip_longest`. Test added with a first name matching three columns: `[revnue,
categry]` now returns `revenue, category, revenue_usd`.
3. **HAVING-filter suggestions lost saved metrics** — fixed in `f870d06d3f`.
`_get_column_suggestions` takes `include_metrics`, and
`_validate_adhoc_filter_columns` passes `clause == "HAVING"`. Parametrized test
covers HAVING (metric offered) vs WHERE (not).
4. **Unranked context list / no cutoff notice** — fixed in `f870d06d3f`.
Fuzzy candidates lead the list, plus `Showing 10 of N columns; call
get_dataset_info for the full list`. Your `customer_regin` case is now a test.
5. **Escaped/truncated names vs the Tool Result Value Contract** — fixed in
`f870d06d3f`. Dataset-owned names in `dataset_context` are returned verbatim,
bounded by count only. Caller-supplied names echoed in `details` stay escaped
and length-limited.
6. **`update_chart` rebind path missing the access check** — fixed in
`f870d06d3f`, in this PR rather than a follow-up. Parametrized test covers
granted / denied / raises, and asserts the target's `table_name` is absent from
the denied response.
7. **Access check overstated in the PR body** — fixed in `f870d06d3f`. The
body now calls it hardening, names `DatasourceFilter` as the existing stock
protection, and keeps it for the guest-token allowlist and
broader-custom-filter cases. The denied test cases carry your comment about
`find_by_id` simulating a dataset the DAO filter admits but `raise_for_access`
denies.
8. **Whole "Did you mean" sentence sanitized as one string** — fixed in
`f870d06d3f`. Candidates are a list template var, so each name gets its own
budget. Separate template keys for the candidate / no-candidate cases keep
`_validate_saved_metrics`' improved wording. Test asserts three ~64-char
candidates survive intact with the `?`, and that `data:revenue` filters only
itself.
9. **Placeholder error with 4 fields overwritten** — fixed in `f870d06d3f`.
New `ChartErrorBuilder.multiple_columns_not_found_error`; `"multiple requested
columns"` is gone. The bounded `DatasetContext` is built in
`_bounded_error_context`, and `_sanitize_user_input` is no longer imported
outside its module.
10. **One column referenced twice reported as "Multiple"** — fixed in
`f870d06d3f`. The branch counts distinct names.
11. **Docs section incomplete** — fixed in `f870d06d3f`. All four tools
named, "saved metrics — both names and expressions", the shared cap, and
promoted to H2.
12. **Stale `sum_boys` comments** — fixed in `f870d06d3f`. Both now say "did
you mean revenue?".
13. **`invalid_saved_metric` rewording untested** — fixed in `f870d06d3f`.
`test_nonexistent_saved_metric_fails` asserts `Available saved metrics:
TotalRevenue` and no `Did you mean:` wrapper. Called out in the PR body.
14. **Test nits and gaps** — fixed in `f870d06d3f`. Empty-schema case
annotated with why it is correctly plural; sanitizer-internal and
`suggestions[-1]` asserts replaced with structural/membership ones; the
candidate-sanitization test now fails on master; added the fail-closed case
where `has_dataset_access` raises, and the >3-candidate multi-column case.
Verification: `pytest tests/unit_tests/mcp_service/` — 6416 passed, 4
skipped. The 4 failures in `test_mcp_caching.py` and `test_worker.py` reproduce
unchanged on `origin/master` in a clean worktree, so they are not from this
branch. Focused run over the chart/explore/validation/utils trees after merging
master: 1061 passed. `pre-commit run` on the staged changed files: all hooks
pass, including mypy, ruff and pylint.
--
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]
