-
Notifications
You must be signed in to change notification settings - Fork 30
feat: warn on fan-out risk in build_v3 SQL generation #2346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,19 +18,24 @@ | |
| DJError, | ||
| DJException, | ||
| DJInvalidInputException, | ||
| DJWarning, | ||
| ErrorCode, | ||
| ) | ||
| from datajunction_server.construction.build_v3.materialization import ( | ||
| get_table_reference_parts_with_materialization, | ||
| ) | ||
| from datajunction_server.construction.build_v3.types import ( | ||
| BuildContext, | ||
| DecomposedMetricInfo, | ||
| DimensionRef, | ||
| GrainGroup, | ||
| JoinPath, | ||
| ResolvedDimension, | ||
| ) | ||
| from datajunction_server.database.dimensionlink import DimensionLink | ||
| from datajunction_server.database.node import Node, NodeType | ||
| from datajunction_server.models.dimensionlink import JoinCardinality | ||
| from datajunction_server.sql.decompose import is_duplication_invariant | ||
| from datajunction_server.sql.parsing import ast | ||
| from datajunction_server.sql.parsing.backends.antlr4 import parse | ||
| from datajunction_server.utils import SEPARATOR | ||
|
|
@@ -1023,3 +1028,116 @@ def build_join_clause( | |
| ) | ||
|
|
||
| return join | ||
|
|
||
|
|
||
| # Fan-out cardinalities (default MANY_TO_ONE is safe). See check_fanout_safety. | ||
| UNSAFE_JOIN_CARDINALITIES = frozenset( | ||
| {JoinCardinality.ONE_TO_MANY, JoinCardinality.MANY_TO_MANY}, | ||
| ) | ||
|
|
||
|
|
||
| def find_unsafe_cardinality_links( | ||
| resolved_dimensions: list[ResolvedDimension], | ||
| ) -> list[DimensionLink]: | ||
| """Return the emitted join-path links whose cardinality can fan out.""" | ||
| unsafe: list[DimensionLink] = [] | ||
| for resolved_dim in resolved_dimensions: | ||
| if resolved_dim.is_local or not resolved_dim.join_path: | ||
| continue | ||
| for link in resolved_dim.join_path.links: | ||
| if link.join_cardinality in UNSAFE_JOIN_CARDINALITIES: | ||
| unsafe.append(link) | ||
| return unsafe | ||
|
|
||
|
|
||
| def _nondecomposable_inflates(info: DecomposedMetricInfo) -> bool: | ||
| """ | ||
| Whether a non-decomposable metric is inflated by row duplication. Holistic | ||
| aggregations (MEDIAN, PERCENTILE, ARRAY_AGG) depend on the full multiset of | ||
| rows, so duplication distorts them; argmax-style MAX_BY/MIN_BY are immune. | ||
| """ | ||
| for func in info.derived_ast.find_all(ast.Function): | ||
| func_class = func.function() | ||
| if func_class is None or not func_class.is_aggregation: | ||
| continue | ||
| if not is_duplication_invariant(func_class): | ||
| return True | ||
| return False | ||
|
|
||
|
|
||
| def check_fanout_safety( | ||
| grain_group: GrainGroup, | ||
| unsafe_links: list[DimensionLink], | ||
| ) -> DJWarning | None: | ||
| """ | ||
| Return a fan-out warning if this grain group aggregates a duplication-sensitive | ||
| metric across a fanning join. | ||
|
|
||
| A ONE_TO_MANY/MANY_TO_MANY link duplicates fact rows. A metric is inflated when | ||
| its result depends on how many times a row appears: additive re-aggregation (a | ||
| SUM merge, used by SUM/COUNT/AVG/...) and holistic aggregations applied over the | ||
| raw rows (MEDIAN, ARRAY_AGG, ...). MIN/MAX, COUNT(DISTINCT), and argmax-style | ||
| MAX_BY/MIN_BY are immune. We warn rather than error: the user may know the data | ||
| does not fan out. | ||
| """ | ||
| if not unsafe_links: | ||
| return None | ||
|
|
||
| # Decomposable metrics with a SUM merge. | ||
| inflated_metrics: set[str] = { | ||
| metric_node.name | ||
| for metric_node, component in grain_group.components | ||
| if component.merge == "SUM" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it only "SUM" where there's a fan-out risk? Does this cover cases like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| # Non-decomposable holistic metrics (MEDIAN, ARRAY_AGG, ...). | ||
| inflated_metrics.update( | ||
| info.metric_node.name | ||
| for info in grain_group.non_decomposable_metrics | ||
| if _nondecomposable_inflates(info) | ||
| ) | ||
|
|
||
| if not inflated_metrics: | ||
| return None | ||
|
|
||
| metric_names = sorted(inflated_metrics) | ||
| link_descriptions = sorted( | ||
| { | ||
| f"{link.node_revision.name} -> {link.dimension.name} " | ||
| f"({JoinCardinality(link.join_cardinality).value})" | ||
| for link in unsafe_links | ||
| }, | ||
| ) | ||
| metrics_str = ", ".join(metric_names) | ||
| links_str = "; ".join(link_descriptions) | ||
| message = ( | ||
| f"Possible fan-out: metric(s) {metrics_str} aggregate across a fan-out " | ||
| f"link [{links_str}] when joining {grain_group.parent_node.name} to the " | ||
| f"requested dimensions. One fact row matches many dimension rows, so the " | ||
| f"result may be inflated by duplicated fact rows. If a filter constrains " | ||
| f"the relationship to be effectively one-to-one this result is correct; " | ||
| f"otherwise remove the offending dimension(s), or correct the link's " | ||
| f"cardinality if the relationship is not actually one-to-many or " | ||
| f"many-to-many." | ||
| ) | ||
| logger.warning( | ||
| "[BuildV3] fan-out risk: metrics=%s links=%s parent=%s", | ||
| metrics_str, | ||
| links_str, | ||
| grain_group.parent_node.name, | ||
| extra={ | ||
| "fanout_metrics": metric_names, | ||
| "fanout_links": link_descriptions, | ||
| "fanout_parent": grain_group.parent_node.name, | ||
| "error_code": ErrorCode.FANOUT_RISK.name, | ||
| }, | ||
| ) | ||
| return DJWarning( | ||
| code=ErrorCode.FANOUT_RISK, | ||
| message=message, | ||
| debug={ | ||
| "metrics": metric_names, | ||
| "links": link_descriptions, | ||
| "parent": grain_group.parent_node.name, | ||
| }, | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this exposes
FANOUT_RISKin the GraphQLErrorCodeenum, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see, if it's v2 then there's no need to add it for now. I think I forgot about this factor.