-
Notifications
You must be signed in to change notification settings - Fork 10
Make dataset problem listings use category labels rather than ids #281
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 |
|---|---|---|
|
|
@@ -40,6 +40,76 @@ class InstanceProblemInterface: | |
| class DatasetDimensionCoordinateType: | ||
| dimension: str = sb.field(description='Dimension column identifier in the dataset.') | ||
| category: str = sb.field(description='Category identifier within the dimension.') | ||
| dimension_label: str = sb.field( | ||
| description="The dimension's label in the active language; falls back to the identifier when unresolvable." | ||
| ) | ||
| category_label: str = sb.field( | ||
| description="The category's label in the active language; falls back to the identifier when unresolvable." | ||
| ) | ||
|
|
||
|
|
||
| #: ``(dataset_uuid, dimension column, category identifier) -> (dimension label, category label)``. | ||
| type CoordinateLabels = dict[tuple[UUID | None, str, str], tuple[str, str]] | ||
|
|
||
|
|
||
| def build_coordinate_labels(violations: Iterable[RuleViolation]) -> CoordinateLabels: | ||
| """ | ||
| Resolve localized labels for the dimension coordinates named by ``violations``. | ||
|
|
||
| Violations are persisted with identifiers only: a materialization is shared | ||
| across users and languages, and ``RuleViolation.key`` diffs on the identifier | ||
| coordinates at edit time. Labels are therefore a presentation concern resolved | ||
| here, in the active language, rather than baked into the stored payload. | ||
|
|
||
| One query pass over the datasets involved, so a violation list costs a fixed | ||
| number of queries rather than one per coordinate. | ||
| """ | ||
| from kausal_common.datasets.models import Dataset, DatasetSchemaDimension, DimensionScope | ||
|
|
||
| dataset_uuids = {violation.dataset_uuid for violation in violations if violation.dataset_uuid is not None} | ||
| if not dataset_uuids: | ||
| return {} | ||
| datasets = list( | ||
| Dataset.objects | ||
| .filter(uuid__in=dataset_uuids) | ||
| .select_related('schema') | ||
| .only('uuid', 'schema', 'scope_content_type', 'scope_id') | ||
| ) | ||
| labels: CoordinateLabels = {} | ||
| for dataset in datasets: | ||
| schema = dataset.schema | ||
| if schema is None or dataset.scope_id is None: | ||
| continue | ||
| # The dataframe column is DatasetSchemaDimension.column_name when set and the | ||
| # scoped dimension identifier otherwise -- the same rule the evaluator applies | ||
| # when it records the coordinate. | ||
| scopes = { | ||
| scope.dimension_id: scope | ||
| for scope in DimensionScope.objects | ||
| .filter( | ||
| scope_content_type=dataset.scope_content_type, | ||
| scope_id=dataset.scope_id, | ||
| dimension_id__in=schema.dimensions.values_list('dimension_id', flat=True), | ||
| ) | ||
| .select_related('dimension') | ||
| .prefetch_related('dimension__categories') | ||
| } | ||
| for schema_dimension in DatasetSchemaDimension.objects.filter(schema=schema).only('dimension_id', 'column_name'): | ||
| scope = scopes.get(schema_dimension.dimension_id) | ||
| if scope is None: | ||
| continue | ||
| column = schema_dimension.column_name or scope.identifier | ||
| if not column: | ||
| continue | ||
| dimension_label = scope.dimension.name_i18n or column | ||
| for category in scope.dimension.categories.all(): | ||
| if category.identifier is None: | ||
| continue | ||
| labels[(dataset.uuid, column, category.identifier)] = ( | ||
| dimension_label, | ||
| category.label_i18n or category.identifier, | ||
| ) | ||
| return labels | ||
|
|
||
|
|
||
| @sb.type( | ||
|
|
@@ -59,7 +129,18 @@ class DatasetValidationViolationType(InstanceProblemInterface): | |
| requirement_group: str | None = sb.field(description='Named required-combination group, when applicable.') | ||
|
|
||
| @classmethod | ||
| def from_violation(cls, violation: RuleViolation) -> Self: | ||
| def from_violation(cls, violation: RuleViolation, labels: CoordinateLabels | None = None) -> Self: | ||
| labels = labels if labels is not None else {} | ||
|
Comment on lines
+132
to
+133
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.
When clients request the new fields through AGENTS.md reference: AGENTS.md:L221-L221 Useful? React with 👍 / 👎. |
||
|
|
||
| def coordinate(dimension: str, category: str) -> DatasetDimensionCoordinateType: | ||
| dimension_label, category_label = labels.get((violation.dataset_uuid, dimension, category), (dimension, category)) | ||
| return DatasetDimensionCoordinateType( | ||
| dimension=dimension, | ||
| category=category, | ||
| dimension_label=dimension_label, | ||
| category_label=category_label, | ||
| ) | ||
|
|
||
| return cls( | ||
| code=violation.kind, | ||
| message=violation.message, | ||
|
|
@@ -71,10 +152,7 @@ def from_violation(cls, violation: RuleViolation) -> Self: | |
| dataset_uuid=violation.dataset_uuid, | ||
| dataset_identifier=violation.dataset_identifier, | ||
| years=list(violation.years), | ||
| coordinates=[ | ||
| DatasetDimensionCoordinateType(dimension=dimension, category=category) | ||
| for dimension, category in violation.categories.items() | ||
| ], | ||
| coordinates=[coordinate(dimension, category) for dimension, category in violation.categories.items()], | ||
| combination_ids=list(violation.combination_ids), | ||
| requirement_group=violation.requirement_group, | ||
| ) | ||
|
|
@@ -92,4 +170,6 @@ class DatasetValidationViolationsType: | |
|
|
||
| @classmethod | ||
| def from_violations(cls, violations: Iterable[RuleViolation]) -> Self: | ||
| return cls(violations=[DatasetValidationViolationType.from_violation(violation) for violation in violations]) | ||
| found = list(violations) | ||
| labels = build_coordinate_labels(found) | ||
| return cls(violations=[DatasetValidationViolationType.from_violation(violation, labels) for violation in found]) | ||
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.
For an instance-wide violation list spanning multiple datasets, this queryset is executed once per dataset by the enclosing loop, its category prefetch adds another query per dataset, and the later
DatasetSchemaDimensionlookup adds a third. Thus the advertised fixed query cost is actually at least3N + 1forNaffected datasets, which can make the editor's problem-list resolver issue dozens of queries; load scopes, categories, and schema dimensions in bulk for all collected datasets.Useful? React with 👍 / 👎.