YuriyKrasilnikov commented on PR #37516:
URL: https://github.com/apache/superset/pull/37516#issuecomment-4969395603
Pushed two follow-up fixes after reviewing the latest CodeAnt
comments and tracing both findings through the execution, cache, and response
contracts.
The first fix separates a contribution total's internal dependency from its
public producer response. The processor previously normalized the producer
into
an all-record totals query, acquired it once, and reused that acquisition as
the
producer's response. That reuse is only valid when the normalized dependency
and
the requested producer have the same acquisition identity. With a row limit,
offset, ordering, series limit, annotations, or a result type with its own
query
preparation (`SAMPLES` / `DRILL_DETAIL`), the public response could therefore
silently contain the internal dependency result instead of the result
requested
for that producer.
The updated contract is:
- the contribution dependency is a canonical `FULL` all-record query;
- the public producer is acquired independently when its execution or result
preparation differs from that dependency;
- exact `FULL`, `RESULTS`, and `POST_PROCESSED` producer acquisitions can
still be
reused because their acquisition identity is unchanged;
- dependency and producer cache identities remain distinct when their queries
differ;
- a failed dependency fails the contribution consumer without replacing or
suppressing the producer's own response;
- neither the consumer nor producer `QueryObject` is mutated.
The second fix handles a partially resolved native annotation request
explicitly.
`AnnotationLayerDAO.find_by_ids()` may return fewer objects than requested;
direct
dictionary indexing then raised `KeyError`, which escaped as a 500. A
missing layer
now raises `QueryObjectValidationError` with the layer ID and reference
name, and
the existing chart-data command/API mapping returns a controlled 400. I also
added
the explicit type for the initially empty annotation payload; the other
suggested
local annotations are already inferred and the changed files pass MyPy.
Regression coverage now includes exact reuse in both materialized and
cache-only
modes, separate acquisition for normalized and specially prepared producers,
cache-key separation, dependency failure, no-mutation assertions, a partial
annotation DAO result, and the API 400 contract.
Verification:
- 137 backend tests passed across the processor, chart-data command/API,
currency,
and dependency suites;
- the final focused processor/API rerun passed at 118 tests;
- the table `buildQuery` suite passed at 25 tests;
- changed-file Ruff, MyPy, and Pylint passed (Pylint: 10.00/10);
- repository-wide Prettier, Oxlint, frontend custom rules, Stylelint, docs
ESLint,
backend MyPy, metadata validation, feature-flag sync, and Zizmor passed.
The repository-wide Ruff/Pylint hooks still report only the unrelated
existing
`execute_sql.py` C901 finding and migration W9001 warnings.
--
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]
