aminghadersohi commented on PR #44349: URL: https://github.com/apache/superset/pull/44349#issuecomment-5703096356
Both suggestions were valid and both are fixed. Neither was pre-existing — the branch introduced both conditions, so they were in scope for it. **Unused scope query under lock** — confirmed. `_evidence_predicates` accepted `scope_entities` and never referenced it; only `_duplicate_predicates` and `_operational_predicates` consume it, via `_repeats_an_earlier_block`, the one predicate that binds a literal entity scope. The evidence category's guards are all correlated per-row probes, so the `DISTINCT entity_type, entity_uuid` lookup was dead work on the locked path — which is the path this change exists to keep short. `master` has no scope lookup at all, so the branch added it. Categories now declare whether their predicates take a scope, and the kwarg is omitted rather than passed empty, so a builder that needs a scope can't silently receive an absent one. The unused parameter is gone from `_evidence_predicates`. The re-check SQL is unchanged. Compiling each category's predicates under both the MySQL and PostgreSQL dialects renders byte-identical statements before and after, and the old `_evidence_predicates` output was already invariant to whatever `scope_entities` it was handed. The only behavioural delta is one fewer statement under the lock, for one category — strictly less work, never more. **Vacuous test comparison** — confirmed. `_evidence_predicates` never reaches `_repeats_an_earlier_block`, so patching the legacy form left `rewritten["evidence"]` and `legacy["evidence"]` computed by identical code and the assertion could not fail. The docstring's claim to cover "evidence bounds" overstated it. Kept the comparison rather than dropping it, but relabelled: it's a control that asserts the rewrite stays confined to its two categories — if `evidence` ever differed, the rewrite had leaked. The docstring now says it proves isolation, not equivalence, and no longer claims evidence-bound coverage. Unit and integration suites for this area pass unchanged (110 unit, 120 integration, 2 skipped as engine-specific), including the seeded equivalence sweep. -- 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]
