YuriyKrasilnikov commented on PR #37516:
URL: https://github.com/apache/superset/pull/37516#issuecomment-4971921742
Reviewed the three additional CodeAnt comments and addressed the
underlying issues in `1c403bb7b3`.
The contribution cache-identity finding was valid, but I did not add
`contribution_totals_query_index` to `QueryObject.to_dict()`. The index is a
query-plan position, not a semantic identity: moving the same totals producer
would cause an unnecessary cache miss, while placing a different producer at
the same index could still collide. Instead, the canonical totals acquisition
now passes its actual producer cache key into the contribution consumer's
cache
identity. This keeps the consumer key stable when only the helper-query
position
changes and changes it when the selected producer query, datasource, RLS, or
extra cache inputs change. The selector remains internal query-context
planning
metadata and the public `QueryObject` serialization contract is unchanged.
The unhashable extra-cache-key finding was also valid, although it was not a
new
regression from `dict.fromkeys`: the previous `set(extra_cache_keys)` failed
for
the same inputs. `cache_key_wrapper()` accepts arbitrary JSON-serializable
values, so list/dict values are legitimate cache-key components.
Deduplication
is now order-preserving and uses the same canonical JSON identity as final
cache
key generation. I also updated the datasource/explorable type contract from
`Hashable` to `Any` so the annotations match the runtime API instead of
hiding
the mismatch.
The streaming validation finding was correct as well. The command
intentionally
raises `QueryObjectValidationError` when a large CSV export cannot produce
executable SQL, but the exception was raised while constructing the response,
outside the existing synchronous API error mapping. `_get_data_response()`
now
owns that boundary for both command execution and response construction, so
the
client receives a 400 with the validation message instead of a generic 500.
The
streaming command remains independent of HTTP concerns.
Regression coverage verifies producer-position stability, producer-identity
separation, propagation through the acquisition path, list/dict cache-key
deduplication, and the streaming 400 response including its message.
Verification:
- 142 expanded processor/query-object/streaming unit tests passed;
- 12 SqlaTable/Jinja extra-cache-key integration tests passed;
- the final focused rerun passed at 63 tests;
- changed-file Ruff, MyPy, and Pylint passed (Pylint: 10.00/10);
- repository-wide MyPy, Prettier, Oxlint, frontend custom rules, Stylelint,
docs ESLint, frontend type-checking, Helm docs, metadata validation,
feature-flag sync, and Zizmor passed.
--
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]
