aminghadersohi commented on PR #40349:
URL: https://github.com/apache/superset/pull/40349#issuecomment-4567296695
Applied Richard's latest round of review feedback from #40344 and #40348 —
cross-applicable patterns updated here.
Reviewed the two new reviews (`4383793289` on #40344, `4383785331` on
#40348). Assessment per note:
| Richard's note | Applies to tags? | Status |
|---|---|---|
| `_sanitize_log_json` should use `excluded_field_names=frozenset()`
(#40344) | No — tags sanitize simple string fields (`name`, `description`) via
`field_path=("name",)`. Since `"name"` and `"description"` are not in
`LLM_CONTEXT_EXCLUDED_FIELD_NAMES`, both are fully wrapped already. The
exclusion concern only arises for nested dicts whose keys happen to match
operational exclusions. | Already correct |
| `dttm` list value normalization (#40344) | No — `TagFilter.col` is
`Literal["name", "type"]`; there is no datetime filter column. | Not applicable
|
| `get_schema` permission gap for `report` (#40348) | No — `"tag"` is not
currently registered in `_SCHEMA_CORE_FACTORIES`; the permission-check
architecture concern is specific to the report integration. | Not applicable |
| `report/schemas.py` name/description not sanitized (#40348) | Yes — but
already addressed in the prior round (commit `b8d8a87e90`) which added
`_sanitize_tag_info_for_llm_context()`. | Already fixed |
--
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]
