feat: warn on fan-out risk in build_v3 SQL generation#2346
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
dac19a1 to
45954bb
Compare
| inflated: list[tuple[Node, MetricComponent]] = [ | ||
| (metric_node, component) | ||
| for metric_node, component in grain_group.components | ||
| if component.merge == "SUM" |
There was a problem hiding this comment.
Is it only "SUM" where there's a fan-out risk? Does this cover cases like ARRAY_AGG, MEDIAN etc?
There was a problem hiding this comment.
No, you're right that those have the same problem. This check only covers cases where DJ decomposes the aggregation. I think all I can do is switch to an allowlist of known safe ones.
| else: | ||
| cube = None | ||
|
|
||
| if use_materialized and cube and cube.availability: |
There was a problem hiding this comment.
When metrics are served from an already-materialized cube, it looks like we skip the grain-group build, so no fan-out warning is surfaced for that query. I get that fan-out risk is really a property of how the cube was materialized (and there is a warning at materialization time) but if that warning was ignored (or the cube was materialized before this check existed), then every downstream consumer of that cube gets the inflated numbers with no signal. Should we persist the fan-out risk on the cube at materialization and re-surface it on reads, so query-time consumers are warned too?
(That could be a follow-up PR if you'd prefer to keep this one tidy)
There was a problem hiding this comment.
Yes I’ve been thinking about how to deal with this. I think I’ll do it in a follow up pr. I think ideally it would be a ci hook to issue the warning before those changes are merged.
| INVALID_COLUMN | ||
| QUERY_SERVICE_ERROR | ||
| INVALID_ORDER_BY | ||
| FANOUT_RISK |
There was a problem hiding this comment.
It looks like this exposes FANOUT_RISK in the GraphQL ErrorCode enum, but no GraphQL type (e.g., GeneratedSQL) actually surfaces a warnings field, so GraphQL consumers still can't see fan-out warnings.
Is exposing warnings on GraphQL in scope for this PR, or intentionally deferred?
There was a problem hiding this comment.
I think I did this because the graphql endpoint is using the v2 path. Should I add this warning to the v2 path as well?
There was a problem hiding this comment.
Ahh I see, if it's v2 then there's no need to add it for now. I think I forgot about this factor.
Emit a non-blocking FANOUT_RISK warning when a metric query joins across ONE_TO_MANY/MANY_TO_MANY dimension links that could inflate an additive (SUM) merge. A dj.sql.fanout_warnings counter is emitted alongside the existing build telemetry.
The guard keyed only on an additive (SUM) merge, so it missed holistic aggregations that are inflated by row duplication: MEDIAN, PERCENTILE, ARRAY_AGG, etc. check_fanout_safety now also inspects grain_group.non_decomposable_metrics and warns on them
974e2d8 to
4b374b1
Compare
| assert is_duplication_invariant(dj_functions.ApproxCountDistinct) is True | ||
| # Derived from a SUM merge -> inflates. | ||
| assert is_duplication_invariant(dj_functions.Sum) is False | ||
| assert is_duplication_invariant(dj_functions.Count) is False |
There was a problem hiding this comment.
Should this also include a test for count distinct? I actually wonder if count distinct also ends up in this "inflates" bucket even though it shouldn't...
2 participants
Emit a non-blocking FANOUT_RISK warning when a metric query joins across ONE_TO_MANY/MANY_TO_MANY dimension links that could inflate an additive (SUM) merge.
A dj.sql.fanout_warnings counter is emitted alongside the existing build telemetry.
See issue #1531