From 36446d7be43bc3b0ef3d64151458d021a85520d1 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Fri, 11 Sep 2026 20:28:32 +0100 Subject: [PATCH 01/11] feat: group rejections for mandatory primary keys --- src/dve/core_engine/backends/base/rules.py | 115 +++++++++++++++--- .../backends/implementations/duckdb/rules.py | 40 ++++++ .../backends/implementations/spark/rules.py | 7 ++ .../core_engine/backends/metadata/rules.py | 10 ++ src/dve/core_engine/type_hints.py | 4 +- src/dve/pipeline/pipeline.py | 50 ++++++-- tests/features/flights.feature | 56 +++++++++ tests/features/steps/steps_pipeline.py | 11 +- tests/testdata/flights/flights.dischema.json | 23 +++- .../flights/invalid_flight_destination.xml | 49 ++++++++ tests/testdata/flights/only_country_id.xml | 5 + 11 files changed, 330 insertions(+), 40 deletions(-) create mode 100644 tests/testdata/flights/invalid_flight_destination.xml create mode 100644 tests/testdata/flights/only_country_id.xml diff --git a/src/dve/core_engine/backends/base/rules.py b/src/dve/core_engine/backends/base/rules.py index 1340e32..a6413c4 100644 --- a/src/dve/core_engine/backends/base/rules.py +++ b/src/dve/core_engine/backends/base/rules.py @@ -28,6 +28,7 @@ CopyEntity, DeferredFilter, EntityRemoval, + GroupIdentification, HeaderJoin, ImmediateFilter, InnerJoin, @@ -344,6 +345,14 @@ def remove_orphans(self, entities: Entities, *, config: OrphanRemoval) -> Iterat """ raise NotImplementedError + @abstractmethod + def check_mandatory_group(self, entities: Entities, *, config: GroupIdentification) -> Iterator: + """ + Check that a mandatory key in an entity has at least one valid entry in the all the child + entities. + """ + raise NotImplementedError + @abstractmethod def union(self, entities: Entities, *, config: TableUnion) -> Messages: """Union two entities together, taking the columns from each by name. @@ -436,36 +445,110 @@ def process_node( code=node.orphaned_records_error_code, message=node.orphaned_records_error_message, location=location, - ) - ) + ), + ), ) for record in _orph_records: - msg_writer.write_queue.put([ + msg_writer.write_queue.put( + [ + FeedbackMessage( + entity=current_entity_name, + record=record, # type: ignore + error_location=location, + error_message=node.orphaned_records_error_message, + failure_type="record", + error_type="record", + error_code=node.orphaned_records_error_code, + reporting_field=location, + category="Parent Missing", + ) + ] + ) + + if node.children: + for child_node in node.children: + process_node(child_node, current_entity_name, orph_messages) + + for root_node in entity_hierarchy.entity_trees.values(): + process_node(root_node, parent_entity_name=None) + + _orph_rel = entities.get(ORPHANED_RECORD_ENTITY_NAME) + if _orph_rel is not None: + del entities[ORPHANED_RECORD_ENTITY_NAME] + + entities.update(entities) + + return [] + + def identify_and_remove_missing_mandatory_groups( + self, + working_directory: URI, + entities: Entities, + entity_hierarchy: EntityHierarchy, + key_fields: Optional[dict[str, list[str]]] = None, + ) -> Messages: + """ + Identify that an entity with a mandatory key has at least one valid child record. + """ + + def process_node( + node: HierarchyNode | ChildHierarchyNode, + parent_entity_name: Optional[EntityName], + ): + """Recursive helper to process a node and its children.""" + current_entity_name = node.entity_name + + if isinstance(node, ChildHierarchyNode) and parent_entity_name is not None: + self.logger.info( + f"Identifying that {current_entity_name} has at least 1 valid child record" + ) # pylint: disable=C0301 + + join_expr = " AND ".join( + f"{parent_entity_name}.{k} = {current_entity_name}.{v}" + for k, v in node.join_fields.items() + ) + + with BackgroundMessageWriter( + working_directory=working_directory, + dve_stage=self.__stage_name__, + key_fields=key_fields, + logger=self.logger, + ) as msg_writer: + location = list(node.join_fields.values())[0] + missing_children_records = self.check_mandatory_group( + entities=entities, + config=GroupIdentification( + entity_name=parent_entity_name, + target_name=node.entity_name, + join_condition=join_expr, + mandatory=node.mandatory, # type: ignore + ), + ) + for record in missing_children_records: + msg_writer.write_queue.put( + [ FeedbackMessage( - entity=current_entity_name, + entity=parent_entity_name, record=record, # type: ignore error_location=location, - error_message=node.orphaned_records_error_message, - failure_type="record", - error_type="record", - error_code=node.orphaned_records_error_code, + error_message=node.no_valid_records_error_message, + failure_type="submission" if node.mandatory else "record", + error_type="submission" if node.mandatory else "record", + error_code=node.no_valid_records_error_code, reporting_field=location, - category="Parent Missing", + category="Children missing", + is_informational=not node.mandatory, # type: ignore ) - ]) + ] + ) if node.children: for child_node in node.children: - process_node(child_node, current_entity_name, orph_messages) - + process_node(child_node, current_entity_name) for root_node in entity_hierarchy.entity_trees.values(): process_node(root_node, parent_entity_name=None) - _orph_rel = entities.get(ORPHANED_RECORD_ENTITY_NAME) - if _orph_rel: - del entities[ORPHANED_RECORD_ENTITY_NAME] - entities.update(entities) return [] diff --git a/src/dve/core_engine/backends/implementations/duckdb/rules.py b/src/dve/core_engine/backends/implementations/duckdb/rules.py index 4479846..0fb5dd7 100644 --- a/src/dve/core_engine/backends/implementations/duckdb/rules.py +++ b/src/dve/core_engine/backends/implementations/duckdb/rules.py @@ -43,6 +43,7 @@ Aggregation, AntiJoin, ConfirmJoinHasMatch, + GroupIdentification, HeaderJoin, ImmediateFilter, InnerJoin, @@ -450,6 +451,45 @@ def remove_orphans(self, entities: DuckDBEntities, *, config: OrphanRemoval) -> orphan_rel.filter(f"entity_name = '{config.entity_name}'") ) + def check_mandatory_group( + self, entities: DuckDBEntities, *, config: GroupIdentification + ) -> Iterator: + """ + Check that a mandatory key in an entity has at least one valid entry in the all the + child entities. + """ + source_rel: DuckDBPyRelation = entities[config.entity_name] + source_rel = source_rel.set_alias(config.entity_name) + target_rel: DuckDBPyRelation = entities[config.target_name] + target_rel = target_rel.set_alias(config.target_name) + + source_columns = [f"{config.entity_name}.{c.strip()}" for c in source_rel.columns] + _pk, fk = config.join_condition.split("=") + + joined_rel = source_rel.join(target_rel, config.join_condition, "left").select( + *source_columns, + ColumnExpression(fk.strip()).alias("fk"), + ConstantExpression(config.mandatory).alias("mandatory"), + ) + + missing_children_rel = joined_rel.filter("fk IS NULL") + filtered_rel = joined_rel.filter("fk IS NOT NULL and not mandatory").select( + StarExpression(exclude=["fk", "mandatory"]) + ) + + _no_valid_child_records: tuple[int] = missing_children_rel.count("*").fetchone() # type: ignore # pylint: disable=C0301 + if _no_valid_child_records: + _no_valid_children = _no_valid_child_records[0] + else: + _no_valid_children = 0 + self.logger.info( + f"Found {_no_valid_children} records with no valid children in {config.entity_name}." + ) # pylint: disable=C0301 + + entities[config.entity_name] = filtered_rel + + return duckdb_rel_to_dictionaries(missing_children_rel) + def union(self, entities: DuckDBEntities, *, config: TableUnion) -> Messages: """Union two entities together, taking the columns from each by name. diff --git a/src/dve/core_engine/backends/implementations/spark/rules.py b/src/dve/core_engine/backends/implementations/spark/rules.py index ff15b52..cec1156 100644 --- a/src/dve/core_engine/backends/implementations/spark/rules.py +++ b/src/dve/core_engine/backends/implementations/spark/rules.py @@ -34,6 +34,7 @@ ColumnAddition, ColumnRemoval, ConfirmJoinHasMatch, + GroupIdentification, HeaderJoin, ImmediateFilter, InnerJoin, @@ -384,6 +385,12 @@ def remove_orphans( # TODO - implement for spark raise NotImplementedError + def check_mandatory_group( + self, entities: SparkEntities, *, config: GroupIdentification + ) -> Iterator: + # TODO - implement for spark + raise NotImplementedError + def filter(self, entities: SparkEntities, *, config: ImmediateFilter) -> Messages: """Filter an entity immediately, and do not emit any messages. diff --git a/src/dve/core_engine/backends/metadata/rules.py b/src/dve/core_engine/backends/metadata/rules.py index 9b96a14..4c26213 100644 --- a/src/dve/core_engine/backends/metadata/rules.py +++ b/src/dve/core_engine/backends/metadata/rules.py @@ -33,6 +33,7 @@ "CopyEntity", "DeferredFilter", "EntityRemoval", + "GroupIdentification", "HeaderJoin", "ImmediateFilter", "InnerJoin", @@ -40,6 +41,7 @@ "OneToOneJoin", "OneToOneJoin", "OrphanIdentification", + "OrphanRemoval", "ParentMetadata", "RenameEntity", "Rule", @@ -553,6 +555,7 @@ class OrphanIdentification(AbstractConditionalJoin): """ + Step = Union[AbstractStep, Literal["sync"]] """A step within a rule. This is either a rule config or the literal string 'sync'.""" @@ -564,6 +567,13 @@ class OrphanRemoval(BaseStep): """The reporting information for the row removal.""" +class GroupIdentification(AbstractConditionalJoin): + """Identify mandatory records which do not have any valid child records""" + + mandatory: bool + """Whether the primary key is mandatory and whether the record should be stripped.""" + + class Rule(BaseModel): """A rule, made up of multiple steps.""" diff --git a/src/dve/core_engine/type_hints.py b/src/dve/core_engine/type_hints.py index e369ff4..48d9eeb 100644 --- a/src/dve/core_engine/type_hints.py +++ b/src/dve/core_engine/type_hints.py @@ -133,7 +133,9 @@ """A string indicating the field that the error pertains to.""" FieldValue = Optional[Any] """The value that caused the error.""" -ErrorCategory = Literal["Blank", "Wrong format", "Bad value", "Bad file", "Parent Missing"] +ErrorCategory = Literal[ + "Blank", "Wrong format", "Bad value", "Bad file", "Parent Missing", "Children missing" +] """A string indicating the category of the error.""" RecordIndex = Optional[int] """The record index that the error relates to (if applicable)""" diff --git a/src/dve/pipeline/pipeline.py b/src/dve/pipeline/pipeline.py index eaf7661..3f214fe 100644 --- a/src/dve/pipeline/pipeline.py +++ b/src/dve/pipeline/pipeline.py @@ -1,4 +1,4 @@ -# pylint: disable=protected-access,too-many-instance-attributes,too-many-arguments,line-too-long +# pylint: disable=protected-access,too-many-instance-attributes,too-many-arguments,line-too-long,too-many-lines """Generic Pipeline object to define how DVE should be interacted with.""" import json @@ -629,18 +629,23 @@ def apply_business_rules( # pylint: disable=R0914 else: self._logger.info(f"Skipping {entity_name}. Marked original.") filtered_entity = entity - projected = self._step_implementations.write_parquet( # type: ignore - filtered_entity, - fh.joinuri( - self.processed_files_path, - submission_info.submission_id, - "business_rules", - entity_name, - ), - ) - entity_manager.entities[entity_name] = self.step_implementations.read_parquet( # type: ignore - projected - ) + # todo - Removing for now as double write causing spark write to crash - look into fix + # todo - main benefit of double write is that the execution plan to be truncated before + # todo - complex joins and checks performed in the orphan and group rejection. + # todo - ideally should only write twice if those steps are actually required. + # projected = self._step_implementations.write_parquet( # type: ignore + # filtered_entity, + # fh.joinuri( + # self.processed_files_path, + # submission_info.submission_id, + # "business_rules", + # entity_name, + # ), + # ) + # entity_manager.entities[entity_name] = self.step_implementations.read_parquet( # type: ignore + # projected + # ) + entity_manager.entities[entity_name] = filtered_entity self.step_implementations.identify_and_remove_orphans( # type: ignore working_directory, @@ -649,6 +654,25 @@ def apply_business_rules( # pylint: disable=R0914 key_fields, ) + self.step_implementations.identify_and_remove_missing_mandatory_groups( # type: ignore + working_directory, + entity_manager.entities, + entity_hierarchy, + key_fields, + ) + + for entity_name, entity in entity_manager.entities.items(): + self._logger.info(f"Writing {entity_name} out to disk.") + self._step_implementations.write_parquet( # type: ignore + entity, + fh.joinuri( + self.processed_files_path, + submission_info.submission_id, + "business_rules", + entity_name, + ), + ) + submission_status.number_of_records = self.get_entity_count( entity=entity_manager.entities[f"""Original{rules.global_variables.get( 'entity', diff --git a/tests/features/flights.feature b/tests/features/flights.feature index ea710e9..f1954c6 100644 --- a/tests/features/flights.feature +++ b/tests/features/flights.feature @@ -114,3 +114,59 @@ Feature: Pipeline tests using the flights dataset # | record_count | 1 | # | number_file_rejections | 0 | # | number_record_rejections | 1 | + + Scenario: A flights submission with no valid airports record on submission + Given I submit the flights file only_country_id.xml for processing + And A duckdb pipeline is configured with schema file 'flights.dischema.json' + And I add initial audit entries for the submission + Then the latest audit record for the submission is marked with processing status file_transformation + When I run the file transformation phase + Then the country entity is stored as a parquet after the file_transformation phase + And the airport entity is stored as a parquet after the file_transformation phase + And the flights entity is stored as a parquet after the file_transformation phase + And the passengers entity is stored as a parquet after the file_transformation phase + And the latest audit record for the submission is marked with processing status data_contract + When I run the data contract phase + Then there are no file rejections from the data_contract phase + And there are no record rejections from the data_contract phase + When I run the business rules phase + Then there are errors with the following details and associated error_count from the business_rules phase + | ErrorType | ErrorCode | error_count | + | submission | C2 | 1 | + When I run the error report phase + Then An error report is produced + # TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets + # And The statistics entry for the submission shows the following information + # | parameter | value | + # | record_count | 1 | + # | number_file_rejections | 0 | + # | number_record_rejections | 1 | + + Scenario: A flights submission with a mixture of group and orphan record rejections + Given I submit the flights file invalid_flight_destination.xml for processing + And A duckdb pipeline is configured with schema file 'flights.dischema.json' + And I add initial audit entries for the submission + Then the latest audit record for the submission is marked with processing status file_transformation + When I run the file transformation phase + Then the country entity is stored as a parquet after the file_transformation phase + And the airport entity is stored as a parquet after the file_transformation phase + And the flights entity is stored as a parquet after the file_transformation phase + And the passengers entity is stored as a parquet after the file_transformation phase + And the latest audit record for the submission is marked with processing status data_contract + When I run the data contract phase + Then there are no file rejections from the data_contract phase + And there are no record rejections from the data_contract phase + When I run the business rules phase + Then there are errors with the following details and associated error_count from the business_rules phase + | ErrorType | Status | ErrorCode | error_count | + | record | error | F2 | 2 | + | record | error | PG1 | 4 | + | record | informational | A1 | 1 | + When I run the error report phase + Then An error report is produced +# TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets +# And The statistics entry for the submission shows the following information +# | parameter | value | +# | record_count | 1 | +# | number_file_rejections | 0 | +# | number_record_rejections | 1 | diff --git a/tests/features/steps/steps_pipeline.py b/tests/features/steps/steps_pipeline.py index c72c873..c71bd45 100644 --- a/tests/features/steps/steps_pipeline.py +++ b/tests/features/steps/steps_pipeline.py @@ -183,8 +183,10 @@ def check_error_record_details_from_service(context: Context, service:str): message_df = load_errors_from_service(processing_path, service) for err_details in error_details: filter_expr, error_count = err_details - assert message_df.filter(filter_expr).shape[0] == error_count - + assert message_df.filter(filter_expr).shape[0] == error_count, message_df.select( + *[pl.col(c) for c in table.headings if c not in ["error_count"]] + ) + @given("A {implementation} pipeline is configured") @given("A {implementation} pipeline is configured with schema file '{schema_file_name}'") @@ -317,8 +319,3 @@ def create_refdata_tables(context: Context, database: str): pipeline._connection.sql(f"ATTACH '{ref_db_file}' AS {database}") for tbl, source in refdata_tables.items(): pipeline._connection.read_parquet(source).to_table(f"{database}.{tbl}") - - - - - diff --git a/tests/testdata/flights/flights.dischema.json b/tests/testdata/flights/flights.dischema.json index faac68f..8ab9dd7 100644 --- a/tests/testdata/flights/flights.dischema.json +++ b/tests/testdata/flights/flights.dischema.json @@ -104,6 +104,17 @@ "category": "Blank", "error_code": "F1" }, + { + "entity": "flights", + "name": "flight_missing_id", + "expression": "lower(destination) IN ('paris', 'madrid')", + "failure_type": "record", + "failure_message": "Record Rejected - {{ destination }} is not a valid destination", + "reporting_field": "flight_id", + "reporting_entity": "flights", + "category": "Bad value", + "error_code": "F2" + }, { "entity": "passengers", "name": "passenger_name_is_null", @@ -125,7 +136,9 @@ }, "mandatory": true, "orphaned_records_error_code": "AG1", - "orphaned_records_error_message": "Group rejected - No valid country group found country" + "orphaned_records_error_message": "Group rejected - No valid country group found country", + "no_valid_records_error_code": "C2", + "no_valid_records_error_message": "File rejected - No valid child entries found for this mandatory key" }, "flights": { "parent_entity": "airport", @@ -134,7 +147,9 @@ }, "mandatory": false, "orphaned_records_error_code": "FG1", - "orphaned_records_error_message": "Group rejected - No valid airport group found for airport" + "orphaned_records_error_message": "Group rejected - No valid airport group found for airport", + "no_valid_records_error_code": "A1", + "no_valid_records_error_message": "Warning - No valid child entries found for this mandatory key" }, "passengers": { "parent_entity": "flights", @@ -143,7 +158,9 @@ }, "mandatory": false, "orphaned_records_error_code": "PG1", - "orphaned_records_error_message": "Group rejected - No valid flight group found for passenger" + "orphaned_records_error_message": "Group rejected - No valid flight group found for passenger", + "no_valid_records_error_code": "F3", + "no_valid_records_error_message": "Warning - No valid child entries found for this mandatory key" } } } \ No newline at end of file diff --git a/tests/testdata/flights/invalid_flight_destination.xml b/tests/testdata/flights/invalid_flight_destination.xml new file mode 100644 index 0000000..134ee89 --- /dev/null +++ b/tests/testdata/flights/invalid_flight_destination.xml @@ -0,0 +1,49 @@ + + + 1 + England + + + 1 + 1 + Heathrow + TW6 1EW + + + 1 + 1 + Mars + + + 1 + 1 + John + + + 1 + 2 + Jane + + + + + 1 + 2 + Venus + + + 2 + 3 + Homer + + + 2 + 4 + Marge + + + + + + + \ No newline at end of file diff --git a/tests/testdata/flights/only_country_id.xml b/tests/testdata/flights/only_country_id.xml new file mode 100644 index 0000000..b0ebd07 --- /dev/null +++ b/tests/testdata/flights/only_country_id.xml @@ -0,0 +1,5 @@ + + + 1 + England + \ No newline at end of file From a1b49bc4930094cafc8305dc05301c5028d05b50 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Mon, 14 Sep 2026 10:57:35 +0100 Subject: [PATCH 02/11] refactor: add initial write back into pipeline --- src/dve/pipeline/pipeline.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/dve/pipeline/pipeline.py b/src/dve/pipeline/pipeline.py index 3f214fe..799b2e9 100644 --- a/src/dve/pipeline/pipeline.py +++ b/src/dve/pipeline/pipeline.py @@ -629,23 +629,18 @@ def apply_business_rules( # pylint: disable=R0914 else: self._logger.info(f"Skipping {entity_name}. Marked original.") filtered_entity = entity - # todo - Removing for now as double write causing spark write to crash - look into fix - # todo - main benefit of double write is that the execution plan to be truncated before - # todo - complex joins and checks performed in the orphan and group rejection. - # todo - ideally should only write twice if those steps are actually required. - # projected = self._step_implementations.write_parquet( # type: ignore - # filtered_entity, - # fh.joinuri( - # self.processed_files_path, - # submission_info.submission_id, - # "business_rules", - # entity_name, - # ), - # ) - # entity_manager.entities[entity_name] = self.step_implementations.read_parquet( # type: ignore - # projected - # ) - entity_manager.entities[entity_name] = filtered_entity + projected = self._step_implementations.write_parquet( # type: ignore + filtered_entity, + fh.joinuri( + self.processed_files_path, + submission_info.submission_id, + "temp_business_rules", + entity_name, + ), + ) + entity_manager.entities[entity_name] = self.step_implementations.read_parquet( # type: ignore + projected + ) self.step_implementations.identify_and_remove_orphans( # type: ignore working_directory, @@ -663,7 +658,7 @@ def apply_business_rules( # pylint: disable=R0914 for entity_name, entity in entity_manager.entities.items(): self._logger.info(f"Writing {entity_name} out to disk.") - self._step_implementations.write_parquet( # type: ignore + final_projection = self._step_implementations.write_parquet( # type: ignore entity, fh.joinuri( self.processed_files_path, @@ -672,6 +667,9 @@ def apply_business_rules( # pylint: disable=R0914 entity_name, ), ) + entity_manager.entities[entity_name] = self.step_implementations.read_parquet( # type: ignore + final_projection + ) submission_status.number_of_records = self.get_entity_count( entity=entity_manager.entities[f"""Original{rules.global_variables.get( From f2a91f36b4d053ae95a41721e7407d1e7950ae74 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:10:32 +0100 Subject: [PATCH 03/11] refactor: add condition to final write to be more performant --- src/dve/core_engine/backends/base/rules.py | 19 ++++++---- src/dve/parser/file_handling/service.py | 7 ++-- src/dve/pipeline/pipeline.py | 42 +++++++++++++++------- src/dve/pipeline/utils.py | 2 +- 4 files changed, 49 insertions(+), 21 deletions(-) diff --git a/src/dve/core_engine/backends/base/rules.py b/src/dve/core_engine/backends/base/rules.py index a6413c4..6247037 100644 --- a/src/dve/core_engine/backends/base/rules.py +++ b/src/dve/core_engine/backends/base/rules.py @@ -391,7 +391,7 @@ def identify_and_remove_orphans( entities: Entities, entity_hierarchy: EntityHierarchy, key_fields: Optional[dict[str, list[str]]] = None, - ) -> Messages: + ) -> tuple[Messages, bool]: """ Identifies and removes orphan records by traversing the EntityHierarchy object. An orphan is a child record whose parent FK does not exist in the parent entity. @@ -469,16 +469,19 @@ def process_node( for child_node in node.children: process_node(child_node, current_entity_name, orph_messages) + processed = False + for root_node in entity_hierarchy.entity_trees.values(): process_node(root_node, parent_entity_name=None) _orph_rel = entities.get(ORPHANED_RECORD_ENTITY_NAME) if _orph_rel is not None: + processed = True del entities[ORPHANED_RECORD_ENTITY_NAME] entities.update(entities) - return [] + return [], processed def identify_and_remove_missing_mandatory_groups( self, @@ -486,7 +489,7 @@ def identify_and_remove_missing_mandatory_groups( entities: Entities, entity_hierarchy: EntityHierarchy, key_fields: Optional[dict[str, list[str]]] = None, - ) -> Messages: + ) -> tuple[Messages, bool]: """ Identify that an entity with a mandatory key has at least one valid child record. """ @@ -494,6 +497,7 @@ def identify_and_remove_missing_mandatory_groups( def process_node( node: HierarchyNode | ChildHierarchyNode, parent_entity_name: Optional[EntityName], + processed: Optional[bool], ): """Recursive helper to process a node and its children.""" current_entity_name = node.entity_name @@ -514,6 +518,7 @@ def process_node( key_fields=key_fields, logger=self.logger, ) as msg_writer: + processed = True location = list(node.join_fields.values())[0] missing_children_records = self.check_mandatory_group( entities=entities, @@ -544,14 +549,16 @@ def process_node( if node.children: for child_node in node.children: - process_node(child_node, current_entity_name) + process_node(child_node, current_entity_name, processed) + + processed = False for root_node in entity_hierarchy.entity_trees.values(): - process_node(root_node, parent_entity_name=None) + process_node(root_node, parent_entity_name=None, processed=processed) entities.update(entities) - return [] + return [], processed # pylint: disable=R0912,R0914 def apply_sync_filters( diff --git a/src/dve/parser/file_handling/service.py b/src/dve/parser/file_handling/service.py index 9ee9d9f..fbdc8ab 100644 --- a/src/dve/parser/file_handling/service.py +++ b/src/dve/parser/file_handling/service.py @@ -273,9 +273,12 @@ def copy_resource(source_uri: URI, target_uri: URI, overwrite: bool = False) -> _transfer_resource(source_uri, target_uri, overwrite, "copy") -def move_resource(source_uri: URI, target_uri: URI, overwrite: bool = False) -> None: - """Move a resource from one location to another.""" +def move_resource(source_uri: URI, target_uri: URI, overwrite: bool = False) -> URI: + """ + Move a resource from one location to another. Returns the target_uri. + """ _transfer_resource(source_uri, target_uri, overwrite, "move") + return target_uri def create_directory(target_uri: URI): diff --git a/src/dve/pipeline/pipeline.py b/src/dve/pipeline/pipeline.py index 799b2e9..326ca7a 100644 --- a/src/dve/pipeline/pipeline.py +++ b/src/dve/pipeline/pipeline.py @@ -642,14 +642,14 @@ def apply_business_rules( # pylint: disable=R0914 projected ) - self.step_implementations.identify_and_remove_orphans( # type: ignore + _, orph_or_group = self.step_implementations.identify_and_remove_orphans( # type: ignore working_directory, entity_manager.entities, entity_hierarchy, key_fields, ) - self.step_implementations.identify_and_remove_missing_mandatory_groups( # type: ignore + _, orph_or_group = self.step_implementations.identify_and_remove_missing_mandatory_groups( # type: ignore working_directory, entity_manager.entities, entity_hierarchy, @@ -657,16 +657,34 @@ def apply_business_rules( # pylint: disable=R0914 ) for entity_name, entity in entity_manager.entities.items(): - self._logger.info(f"Writing {entity_name} out to disk.") - final_projection = self._step_implementations.write_parquet( # type: ignore - entity, - fh.joinuri( - self.processed_files_path, - submission_info.submission_id, - "business_rules", - entity_name, - ), - ) + if orph_or_group: + self._logger.info(f"Writing {entity_name} out to disk.") + final_projection = self._step_implementations.write_parquet( # type: ignore + entity, + fh.joinuri( + self.processed_files_path, + submission_info.submission_id, + "business_rules", + entity_name, + ), + ) + else: + self._logger.info(f"Moving {entity_name} from temp_business_rules to business_rules") + final_projection = fh.move_resource( + source_uri=fh.joinuri( + self.processed_files_path, + submission_info.submission_id, + "temp_business_rules", + entity_name + ), + target_uri=fh.joinuri( + self.processed_files_path, + submission_info.submission_id, + "business_rules", + entity_name + ) + ) + entity_manager.entities[entity_name] = self.step_implementations.read_parquet( # type: ignore final_projection ) diff --git a/src/dve/pipeline/utils.py b/src/dve/pipeline/utils.py index e6122c2..babcd47 100644 --- a/src/dve/pipeline/utils.py +++ b/src/dve/pipeline/utils.py @@ -68,7 +68,7 @@ def unpersist_all_rdds(spark: SparkSession): rdd.unpersist() -def deadletter_file(source_uri: URI) -> None: +def deadletter_file(source_uri: URI) -> URI | None: """Move files that can't be processed to a deadletter location""" try: source_parent: URI = source_uri.rsplit("/", 1)[0] From 4a0288d169a4b6742d53dac3a58e74bcb3725947 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:29:38 +0100 Subject: [PATCH 04/11] style: sonar feedback M --- src/dve/core_engine/backends/base/rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dve/core_engine/backends/base/rules.py b/src/dve/core_engine/backends/base/rules.py index 6247037..741d566 100644 --- a/src/dve/core_engine/backends/base/rules.py +++ b/src/dve/core_engine/backends/base/rules.py @@ -519,7 +519,7 @@ def process_node( logger=self.logger, ) as msg_writer: processed = True - location = list(node.join_fields.values())[0] + location = next(iter(node.join_fields.values())) missing_children_records = self.check_mandatory_group( entities=entities, config=GroupIdentification( From 3114b4fac5a20f3b51d69d41c7947e93f7e0f77f Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Tue, 15 Sep 2026 13:25:38 +0100 Subject: [PATCH 05/11] docs: update jsonschema for entity relationships to include group rej code+message --- .../json_schemas/entity_relationships.schema.json | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/advanced_guidance/json_schemas/entity_relationships.schema.json b/docs/advanced_guidance/json_schemas/entity_relationships.schema.json index 9d4e363..ff35309 100644 --- a/docs/advanced_guidance/json_schemas/entity_relationships.schema.json +++ b/docs/advanced_guidance/json_schemas/entity_relationships.schema.json @@ -25,6 +25,12 @@ }, "missing_parent_id_error_message": { "type": "string" + }, + "no_valid_records_error_code": { + "type": "string" + }, + "no_valid_records_error_message": { + "type": "string" } }, "required": [ From 9863565757b205987deda11b435ce58560f7e6e1 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Tue, 15 Sep 2026 13:38:19 +0100 Subject: [PATCH 06/11] fix: remove childHierarchy object that's no longer valid --- src/dve/core_engine/backends/base/rules.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dve/core_engine/backends/base/rules.py b/src/dve/core_engine/backends/base/rules.py index 362293c..b220631 100644 --- a/src/dve/core_engine/backends/base/rules.py +++ b/src/dve/core_engine/backends/base/rules.py @@ -494,14 +494,14 @@ def identify_and_remove_missing_mandatory_groups( """ def process_node( - node: HierarchyNode | ChildHierarchyNode, + node: HierarchyNode, parent_entity_name: Optional[EntityName], processed: Optional[bool], ): """Recursive helper to process a node and its children.""" current_entity_name = node.entity_name - if isinstance(node, ChildHierarchyNode) and parent_entity_name is not None: + if parent_entity_name is not None: self.logger.info( f"Identifying that {current_entity_name} has at least 1 valid child record" ) # pylint: disable=C0301 From 953f1a3dcaf1264c664fa5e6caed82de91499fbe Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Tue, 15 Sep 2026 14:03:40 +0100 Subject: [PATCH 07/11] fix: change processed in orphan and group rejections to work as intended --- src/dve/core_engine/backends/base/rules.py | 25 +++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/dve/core_engine/backends/base/rules.py b/src/dve/core_engine/backends/base/rules.py index b220631..26de282 100644 --- a/src/dve/core_engine/backends/base/rules.py +++ b/src/dve/core_engine/backends/base/rules.py @@ -397,14 +397,11 @@ def identify_and_remove_orphans( def process_node( node: HierarchyNode, parent_entity_name: Optional[EntityName], - orph_messages: Messages | None = None, - ): + processed: bool = False, + ) -> bool: """Recursive helper to process a node and its children.""" current_entity_name = node.entity_name - if orph_messages is None: - orph_messages = [] - if parent_entity_name is not None: self.logger.info(f"Identifying orphans in {current_entity_name}") @@ -427,6 +424,7 @@ def process_node( self.logger.info( f"Removing records with missing parent from {current_entity_name}" ) + processed = True location = list(node.join_fields.values())[0] with BackgroundMessageWriter( working_directory=working_directory, @@ -466,16 +464,17 @@ def process_node( if node.children: for child_node in node.children: - process_node(child_node, current_entity_name, orph_messages) + processed = process_node(child_node, current_entity_name, processed) + + return processed processed = False for root_node in entity_hierarchy.entity_trees.values(): - process_node(root_node, parent_entity_name=None) + processed = process_node(root_node, parent_entity_name=None, processed=processed) _orph_rel = entities.get(ORPHANED_RECORD_ENTITY_NAME) if _orph_rel is not None: - processed = True del entities[ORPHANED_RECORD_ENTITY_NAME] entities.update(entities) @@ -496,8 +495,8 @@ def identify_and_remove_missing_mandatory_groups( def process_node( node: HierarchyNode, parent_entity_name: Optional[EntityName], - processed: Optional[bool], - ): + processed: bool = False, + ) -> bool: """Recursive helper to process a node and its children.""" current_entity_name = node.entity_name @@ -548,12 +547,14 @@ def process_node( if node.children: for child_node in node.children: - process_node(child_node, current_entity_name, processed) + processed = process_node(child_node, current_entity_name, processed) + + return processed processed = False for root_node in entity_hierarchy.entity_trees.values(): - process_node(root_node, parent_entity_name=None, processed=processed) + processed = process_node(root_node, parent_entity_name=None, processed=processed) entities.update(entities) From 874c0309903408a71a8a5f992d60bff19cdf8335 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Fri, 18 Sep 2026 10:37:15 +0100 Subject: [PATCH 08/11] fix: allow group rejections to work correctly on multiple mandatory nodes --- src/dve/core_engine/backends/base/rules.py | 99 +++--- .../backends/implementations/duckdb/rules.py | 5 +- .../core_engine/backends/metadata/rules.py | 3 - src/dve/pipeline/pipeline.py | 10 +- tests/features/flights.feature | 128 +++---- tests/features/steps/steps_post_pipeline.py | 2 +- tests/testdata/flights/flights.dischema.json | 39 +-- ... flights_data_contract_error_details.json} | 2 +- .../mixture_of_group_rej_and_bi_rej.xml | 314 ------------------ .../flights/multi_node_file_rejection.xml | 87 +++++ ...ation.xml => singular_node_rejections.xml} | 0 11 files changed, 232 insertions(+), 457 deletions(-) rename tests/testdata/flights/{flights_contract_error_details.json => flights_data_contract_error_details.json} (73%) delete mode 100644 tests/testdata/flights/mixture_of_group_rej_and_bi_rej.xml create mode 100644 tests/testdata/flights/multi_node_file_rejection.xml rename tests/testdata/flights/{invalid_flight_destination.xml => singular_node_rejections.xml} (100%) diff --git a/src/dve/core_engine/backends/base/rules.py b/src/dve/core_engine/backends/base/rules.py index ffbfe77..a3ed6a4 100644 --- a/src/dve/core_engine/backends/base/rules.py +++ b/src/dve/core_engine/backends/base/rules.py @@ -493,67 +493,62 @@ def identify_and_remove_missing_mandatory_groups( def process_node( node: HierarchyNode, - parent_entity_name: Optional[EntityName], processed: bool = False, ) -> bool: - """Recursive helper to process a node and its children.""" - current_entity_name = node.entity_name - - if parent_entity_name is not None: - self.logger.info( - f"Identifying that {current_entity_name} has at least 1 valid child record" - ) # pylint: disable=C0301 - - join_expr = " AND ".join( - f"{parent_entity_name}.{k} = {current_entity_name}.{v}" - for k, v in node.join_fields.items() + """Identify at least one valid child for a mandatory entity at a given node.""" + if node.parent_entity is None or not node.mandatory: + return processed + + processed = True + + self.logger.info( + f"Identifying that mandatory entity `{node.parent_entity}` has at least 1 valid child record" # pylint: disable=C0301 + ) + + join_expr = " AND ".join( + f"{node.parent_entity}.{k} = {node.entity_name}.{v}" + for k, v in node.join_fields.items() + ) + + with BackgroundMessageWriter( + working_directory=working_directory, + dve_stage=self.__stage_name__, + key_fields=key_fields, + logger=self.logger, + ) as msg_writer: + location = next(iter(node.join_fields.values())) + missing_children_records = self.check_mandatory_group( + entities=entities, + config=GroupIdentification( + entity_name=node.parent_entity, + target_name=node.entity_name, + join_condition=join_expr, + ), ) - - with BackgroundMessageWriter( - working_directory=working_directory, - dve_stage=self.__stage_name__, - key_fields=key_fields, - logger=self.logger, - ) as msg_writer: - processed = True - location = next(iter(node.join_fields.values())) - missing_children_records = self.check_mandatory_group( - entities=entities, - config=GroupIdentification( - entity_name=parent_entity_name, - target_name=node.entity_name, - join_condition=join_expr, - mandatory=node.mandatory, # type: ignore - ), + for record in missing_children_records: + msg_writer.write_queue.put( + [ + FeedbackMessage( + entity=node.parent_entity, + record=record, # type: ignore + error_location=location, + error_message=node.no_valid_records_error_message, + failure_type="record", + error_type="record", + error_code=node.no_valid_records_error_code, + reporting_field=location, + category="Children missing", + ) + ] ) - for record in missing_children_records: - msg_writer.write_queue.put( - [ - FeedbackMessage( - entity=parent_entity_name, - record=record, # type: ignore - error_location=location, - error_message=node.no_valid_records_error_message, - failure_type="submission" if node.mandatory else "record", - error_type="submission" if node.mandatory else "record", - error_code=node.no_valid_records_error_code, - reporting_field=location, - category="Children missing", - is_informational=not node.mandatory, # type: ignore - ) - ] - ) - - if node.children: - for child_node in node.children: - processed = process_node(child_node, current_entity_name, processed) return processed processed = False - for root_node in entity_hierarchy.entity_trees.values(): - processed = process_node(root_node, parent_entity_name=None, processed=processed) + for tree in entity_hierarchy.entity_trees.values(): + for node in tree.iterate_lowest_descendent_up(): + processed = process_node(node, processed) entities.update(entities) diff --git a/src/dve/core_engine/backends/implementations/duckdb/rules.py b/src/dve/core_engine/backends/implementations/duckdb/rules.py index 0fb5dd7..867b262 100644 --- a/src/dve/core_engine/backends/implementations/duckdb/rules.py +++ b/src/dve/core_engine/backends/implementations/duckdb/rules.py @@ -469,13 +469,10 @@ def check_mandatory_group( joined_rel = source_rel.join(target_rel, config.join_condition, "left").select( *source_columns, ColumnExpression(fk.strip()).alias("fk"), - ConstantExpression(config.mandatory).alias("mandatory"), ) missing_children_rel = joined_rel.filter("fk IS NULL") - filtered_rel = joined_rel.filter("fk IS NOT NULL and not mandatory").select( - StarExpression(exclude=["fk", "mandatory"]) - ) + filtered_rel = joined_rel.filter("fk IS NOT NULL").select(StarExpression(exclude=["fk"])) _no_valid_child_records: tuple[int] = missing_children_rel.count("*").fetchone() # type: ignore # pylint: disable=C0301 if _no_valid_child_records: diff --git a/src/dve/core_engine/backends/metadata/rules.py b/src/dve/core_engine/backends/metadata/rules.py index 4c26213..1b0121e 100644 --- a/src/dve/core_engine/backends/metadata/rules.py +++ b/src/dve/core_engine/backends/metadata/rules.py @@ -570,9 +570,6 @@ class OrphanRemoval(BaseStep): class GroupIdentification(AbstractConditionalJoin): """Identify mandatory records which do not have any valid child records""" - mandatory: bool - """Whether the primary key is mandatory and whether the record should be stripped.""" - class Rule(BaseModel): """A rule, made up of multiple steps.""" diff --git a/src/dve/pipeline/pipeline.py b/src/dve/pipeline/pipeline.py index d4a31b8..750ef8c 100644 --- a/src/dve/pipeline/pipeline.py +++ b/src/dve/pipeline/pipeline.py @@ -545,7 +545,7 @@ def data_contract_step( return processed_files, failed_processing - def apply_business_rules( # pylint: disable=R0914 + def apply_business_rules( # pylint: disable=R0914,R0915 self, submission_info: SubmissionInfo, submission_status: Optional[SubmissionStatus] = None ) -> tuple[SubmissionInfo, SubmissionStatus]: """Apply the business rules to a given submission, the submission may have failed at the @@ -657,6 +657,14 @@ def apply_business_rules( # pylint: disable=R0914 key_fields, ) + # Perform a second time incase the mandatory groups result in new orphans + _, orph_or_group = self.step_implementations.identify_and_remove_orphans( # type: ignore + working_directory, + entity_manager.entities, + entity_hierarchy, + key_fields, + ) + for entity_name, entity in entity_manager.entities.items(): if orph_or_group: self._logger.info(f"Writing {entity_name} out to disk.") diff --git a/tests/features/flights.feature b/tests/features/flights.feature index 80e1b53..75ceec2 100644 --- a/tests/features/flights.feature +++ b/tests/features/flights.feature @@ -22,12 +22,12 @@ Feature: Pipeline tests using the flights dataset And there are no record rejections from the business_rules phase When I run the error report phase Then An error report is produced - # TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets - # And The statistics entry for the submission shows the following information - # | parameter | value | - # | record_count | 1 | - # | number_file_rejections | 0 | - # | number_record_rejections | 0 | + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 0 | + | number_warnings | 0 | Scenario: A flights submission where the root record is rejected Given I submit the flights file missing_country_id.xml for processing @@ -44,21 +44,24 @@ Feature: Pipeline tests using the flights dataset When I run the data contract phase Then there are no file rejections from the data_contract phase And there is 1 record rejection from the data_contract phase + # And there are errors with the following details and associated error_count from the data_contract phase + # | ErrorType | ErrorCode | error_count | + # | record | CountryIdIsMissing | 1 | When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase - | ErrorType | ErrorCode | error_count | - | record | AG1 | 3 | - | record | SG1 | 15 | - | record | FG1 | 10 | - | record | PG1 | 25 | + | ErrorType | ErrorCode | error_count | + | record | AirportHasNoCountry | 3 | + | record | StaffHasNoAirport | 15 | + | record | FlightHasNoAirport | 10 | + | record | PassengerHasNoFlight | 25 | When I run the error report phase Then An error report is produced - # TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets - # And The statistics entry for the submission shows the following information - # | parameter | value | - # | record_count | 1 | - # | number_file_rejections | 0 | - # | number_record_rejections | 1 | + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 54 | + | number_warnings | 0 | Scenario: A flights submission where a child primary key is rejected Given I submit the flights file missing_flight_id.xml for processing @@ -77,20 +80,20 @@ Feature: Pipeline tests using the flights dataset And there are no record rejections from the data_contract phase When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase - | ErrorType | ErrorCode | error_count | - | record | F1 | 1 | - | record | PG1 | 3 | + | ErrorType | ErrorCode | error_count | + | record | FlightIDMissing | 1 | + | record | PassengerHasNoFlight | 3 | When I run the error report phase Then An error report is produced - #TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets - # And The statistics entry for the submission shows the following information - # | parameter | value | - # | record_count | 1 | - # | number_file_rejections | 0 | - # | number_record_rejections | 1 | + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 4 | + | number_warnings | 0 | - Scenario: A flights submission with a mixture of group and record rejections - Given I submit the flights file mixture_of_group_rej_and_bi_rej.xml for processing + Scenario: A flights submission with no valid airports record on submission + Given I submit the flights file only_country_id.xml for processing And A duckdb pipeline is configured with schema file 'flights.dischema.json' And I add initial audit entries for the submission Then the latest audit record for the submission is marked with processing status file_transformation @@ -98,7 +101,6 @@ Feature: Pipeline tests using the flights dataset Then the country entity is stored as a parquet after the file_transformation phase And the airport entity is stored as a parquet after the file_transformation phase And the flights entity is stored as a parquet after the file_transformation phase - And the staff entity is stored as a parquet after the file_transformation phase And the passengers entity is stored as a parquet after the file_transformation phase And the latest audit record for the submission is marked with processing status data_contract When I run the data contract phase @@ -106,22 +108,19 @@ Feature: Pipeline tests using the flights dataset And there are no record rejections from the data_contract phase When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase - | ErrorType | ErrorCode | error_count | - | record | F1 | 1 | - | record | PG1 | 3 | - | record | P1 | 1 | - | record | S1 | 7 | + | ErrorType | ErrorCode | error_count | + | record | CountryHasNoAirport | 1 | When I run the error report phase Then An error report is produced - # TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets - # And The statistics entry for the submission shows the following information - # | parameter | value | - # | record_count | 1 | - # | number_file_rejections | 0 | - # | number_record_rejections | 1 | + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 1 | + | number_warnings | 0 | - Scenario: A flights submission with no valid airports record on submission - Given I submit the flights file only_country_id.xml for processing + Scenario: A flights submission with a rejection on a node with one mandatory node + Given I submit the flights file singular_node_rejections.xml for processing And A duckdb pipeline is configured with schema file 'flights.dischema.json' And I add initial audit entries for the submission Then the latest audit record for the submission is marked with processing status file_transformation @@ -136,19 +135,22 @@ Feature: Pipeline tests using the flights dataset And there are no record rejections from the data_contract phase When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase - | ErrorType | ErrorCode | error_count | - | submission | C2 | 1 | + | ErrorType | Status | ErrorCode | error_count | + | record | error | InvalidFlightDestination | 2 | + | record | error | PassengerHasNoFlight | 4 | + | record | error | AirportHasNoStaff | 1 | + | record | error | CountryHasNoAirport | 1 | When I run the error report phase Then An error report is produced - # TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets - # And The statistics entry for the submission shows the following information - # | parameter | value | - # | record_count | 1 | - # | number_file_rejections | 0 | - # | number_record_rejections | 1 | + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 8 | + | number_warnings | 0 | - Scenario: A flights submission with a mixture of group and orphan record rejections - Given I submit the flights file invalid_flight_destination.xml for processing + Scenario: A flights submission with a rejection on a node with two mandatory nodes + Given I submit the flights file multi_node_file_rejection.xml for processing And A duckdb pipeline is configured with schema file 'flights.dischema.json' And I add initial audit entries for the submission Then the latest audit record for the submission is marked with processing status file_transformation @@ -157,21 +159,23 @@ Feature: Pipeline tests using the flights dataset And the airport entity is stored as a parquet after the file_transformation phase And the flights entity is stored as a parquet after the file_transformation phase And the passengers entity is stored as a parquet after the file_transformation phase + And the passengers entity is stored as a parquet after the file_transformation phase And the latest audit record for the submission is marked with processing status data_contract When I run the data contract phase Then there are no file rejections from the data_contract phase And there are no record rejections from the data_contract phase When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase - | ErrorType | Status | ErrorCode | error_count | - | record | error | F2 | 2 | - | record | error | PG1 | 4 | - # | record | informational | A1 | 1 | + | ErrorType | Status | ErrorCode | error_count | + | record | error | StaffIDMissing | 6 | + | record | error | AirportHasNoStaff | 1 | + | record | error | FlightHasNoAirport | 1 | + | record | error | PassengerHasNoFlight | 1 | When I run the error report phase Then An error report is produced -# TODO - fix the stats calculations as they're currently incorrect for hiearchical datasets -# And The statistics entry for the submission shows the following information -# | parameter | value | -# | record_count | 1 | -# | number_file_rejections | 0 | -# | number_record_rejections | 1 | + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 9 | + | number_warnings | 0 | diff --git a/tests/features/steps/steps_post_pipeline.py b/tests/features/steps/steps_post_pipeline.py index 906445e..6c70174 100644 --- a/tests/features/steps/steps_post_pipeline.py +++ b/tests/features/steps/steps_post_pipeline.py @@ -110,7 +110,7 @@ def check_stats_record(context): stats = ( get_pipeline(context)._audit_tables.get_submission_statistics(sub_info.submission_id).model_dump() ) - assert all([val == stats.get(fld) for fld, val in expected.items()]) + assert all([val == stats.get(fld) for fld, val in expected.items()]), stats @then("the error aggregates are persisted") def check_error_aggregates_persisted(context): diff --git a/tests/testdata/flights/flights.dischema.json b/tests/testdata/flights/flights.dischema.json index 9ea7d07..c862469 100644 --- a/tests/testdata/flights/flights.dischema.json +++ b/tests/testdata/flights/flights.dischema.json @@ -9,7 +9,7 @@ } } }, - "error_details": "flights_contract_error_details.json", + "error_details": "flights_data_contract_error_details.json", "datasets": { "country": { "fields": { @@ -104,6 +104,9 @@ } }, "transformations": { + "parameters": { + "entity": "country" + }, "filters": [ { "entity": "flights", @@ -114,7 +117,7 @@ "reporting_field": "flight_id", "reporting_entity": "flights", "category": "Blank", - "error_code": "F1" + "error_code": "FlightIDMissing" }, { "entity": "flights", @@ -125,7 +128,7 @@ "reporting_field": "flight_id", "reporting_entity": "flights", "category": "Bad value", - "error_code": "F2" + "error_code": "InvalidFlightDestination" }, { "entity": "passengers", @@ -136,7 +139,7 @@ "reporting_field": "passenger_name", "reporting_entity": "passengers", "category": "Blank", - "error_code": "P1" + "error_code": "PassengerNameMissing" }, { "entity": "staff", @@ -147,7 +150,7 @@ "reporting_field": "passenger_name", "reporting_entity": "passengers", "category": "Blank", - "error_code": "S1" + "error_code": "StaffIDMissing" } ] }, @@ -158,10 +161,10 @@ "country_id": "country_id" }, "mandatory": true, - "missing_parent_id_error_code": "AG1", - "missing_parent_id_error_message": "Group rejected - No valid country group found country", - "no_valid_records_error_code": "C2", - "no_valid_records_error_message": "File rejected - No valid child entries found for this mandatory key" + "missing_parent_id_error_code": "AirportHasNoCountry", + "missing_parent_id_error_message": "Record rejected - No valid country id found for airport", + "no_valid_records_error_code": "CountryHasNoAirport", + "no_valid_records_error_message": "Group rejected - Unable to find any valid airports" }, "staff": { "parent_entity": "airport", @@ -169,8 +172,10 @@ "airport_id": "airport_id" }, "mandatory": true, - "missing_parent_id_error_code": "SG1", - "missing_parent_id_error_message": "Group rejected - No valid airport group found for staff" + "missing_parent_id_error_code": "StaffHasNoAirport", + "missing_parent_id_error_message": "Record rejected - No valid airport id found for staff", + "no_valid_records_error_code": "AirportHasNoStaff", + "no_valid_records_error_message": "Group rejected - Airport has no valid staff" }, "flights": { "parent_entity": "airport", @@ -178,10 +183,8 @@ "airport_id": "airport_id" }, "mandatory": false, - "missing_parent_id_error_code": "FG1", - "missing_parent_id_error_message": "Group rejected - No valid airport group found for airport", - "no_valid_records_error_code": "A1", - "no_valid_records_error_message": "Warning - No valid child entries found for this mandatory key" + "missing_parent_id_error_code": "FlightHasNoAirport", + "missing_parent_id_error_message": "Record Rejected - No valid airport found for flight" }, "passengers": { "parent_entity": "flights", @@ -189,10 +192,8 @@ "flight_id": "flight_id" }, "mandatory": false, - "missing_parent_id_error_code": "PG1", - "missing_parent_id_error_message": "Group rejected - No valid flight group found for passenger", - "no_valid_records_error_code": "F3", - "no_valid_records_error_message": "Warning - No valid child entries found for this mandatory key" + "missing_parent_id_error_code": "PassengerHasNoFlight", + "missing_parent_id_error_message": "Record rejected - No valid flight found for passenger" } } } \ No newline at end of file diff --git a/tests/testdata/flights/flights_contract_error_details.json b/tests/testdata/flights/flights_data_contract_error_details.json similarity index 73% rename from tests/testdata/flights/flights_contract_error_details.json rename to tests/testdata/flights/flights_data_contract_error_details.json index f3c7e3f..cb6541e 100644 --- a/tests/testdata/flights/flights_contract_error_details.json +++ b/tests/testdata/flights/flights_data_contract_error_details.json @@ -1,7 +1,7 @@ { "country_id": { "Blank": { - "error_code": "C1", + "error_code": "CountryIdIsMissing", "error_message": "Record Rejected - Country is missing an id" } } diff --git a/tests/testdata/flights/mixture_of_group_rej_and_bi_rej.xml b/tests/testdata/flights/mixture_of_group_rej_and_bi_rej.xml deleted file mode 100644 index 3e40b18..0000000 --- a/tests/testdata/flights/mixture_of_group_rej_and_bi_rej.xml +++ /dev/null @@ -1,314 +0,0 @@ - - - 1 - England - - - 1 - 1 - Heathrow - TW6 1EW - - - 1 - Paris - - - 1 - 1 - John - - - 1 - 2 - Jane - - - 1 - 3 - Peter - - - - - 1 - 2 - Madrid - - - 2 - 4 - Homer - - - 2 - 5 - - - - - 1 - 3 - New York - - - 3 - 6 - Lisa - - - 3 - 7 - Bart - - - 3 - 8 - Maggie - - - - - - - 1 - Alice - Manager - - - 1 - 2 - Bob - Pilot - - - 1 - 3 - Charlie - Ground Crew - - - 1 - 4 - Diana - Security - - - - - 1 - 2 - Gatwick - RH6 0NP - - - 2 - 4 - Amsterdam - - - 4 - 9 - Oliver - - - 4 - 10 - Emily - - - - - 2 - 5 - Rome - - - 5 - 11 - George - - - 5 - 12 - Charlotte - - - 5 - 13 - Harry - - - - - - 2 - 6 - Dubai - - - 6 - 14 - William - - - 6 - 15 - Amelia - - - - - - - 2 - 5 - Edward - Manager - - - 2 - 6 - Fiona - Air Traffic Controller - - - 2 - 7 - Graham - Ground Crew - - - 2 - 8 - Hannah - Security - - - 2 - 9 - Ian - Engineer - - - - - 1 - 3 - Manchester - M90 1QX - - - 3 - 7 - Dublin - - - 7 - 16 - Jack - - - 7 - 17 - Isla - - - - - 3 - 8 - Lisbon - - - 8 - 18 - Thomas - - - 8 - 19 - Grace - - - 8 - 20 - Jacob - - - - - 3 - 9 - Toronto - - - 9 - 21 - Leo - - - 9 - 22 - Sophie - - - - - 3 - 10 - New York - - - 10 - 23 - Daniel - - - 10 - 24 - Ella - - - 10 - 25 - Oscar - - - - - - - 3 - Kevin - Manager - - - 3 - Laura - Pilot - - - 3 - Michael - Air Traffic Controller - - - 3 - Natalie - Ground Crew - - - 3 - Oliver - Security - - - 3 - Paula - Engineer - - - - - diff --git a/tests/testdata/flights/multi_node_file_rejection.xml b/tests/testdata/flights/multi_node_file_rejection.xml new file mode 100644 index 0000000..0958446 --- /dev/null +++ b/tests/testdata/flights/multi_node_file_rejection.xml @@ -0,0 +1,87 @@ + + + 1 + England + + + 1 + 1 + Heathrow + TW6 1EW + + + 1 + 1 + Paris + + + 1 + 1 + John + + + + + + + 1 + 1 + Alice + Manager + + + + + 1 + 2 + Manchester + M90 1QX + + + 2 + 2 + Dublin + + + 2 + 2 + Jack + + + + + + + 3 + Kevin + Manager + + + 3 + Laura + Pilot + + + 3 + Michael + Air Traffic Controller + + + 3 + Natalie + Ground Crew + + + 3 + Oliver + Security + + + 3 + Paula + Engineer + + + + + diff --git a/tests/testdata/flights/invalid_flight_destination.xml b/tests/testdata/flights/singular_node_rejections.xml similarity index 100% rename from tests/testdata/flights/invalid_flight_destination.xml rename to tests/testdata/flights/singular_node_rejections.xml From e41bfd7133600a3052014c13ac24686f44094890 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:05:08 +0100 Subject: [PATCH 09/11] test: fixed flights data contract detail and added additional check in multi node flights test --- tests/features/flights.feature | 10 +++++----- tests/testdata/flights/multi_node_file_rejection.xml | 5 +++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/features/flights.feature b/tests/features/flights.feature index 75ceec2..bc295e3 100644 --- a/tests/features/flights.feature +++ b/tests/features/flights.feature @@ -44,9 +44,9 @@ Feature: Pipeline tests using the flights dataset When I run the data contract phase Then there are no file rejections from the data_contract phase And there is 1 record rejection from the data_contract phase - # And there are errors with the following details and associated error_count from the data_contract phase - # | ErrorType | ErrorCode | error_count | - # | record | CountryIdIsMissing | 1 | + And there are errors with the following details and associated error_count from the data_contract phase + | FailureType | ErrorCode | error_count | + | record | CountryIdIsMissing | 1 | When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase | ErrorType | ErrorCode | error_count | @@ -167,7 +167,7 @@ Feature: Pipeline tests using the flights dataset When I run the business rules phase Then there are errors with the following details and associated error_count from the business_rules phase | ErrorType | Status | ErrorCode | error_count | - | record | error | StaffIDMissing | 6 | + | record | error | StaffIDMissing | 7 | | record | error | AirportHasNoStaff | 1 | | record | error | FlightHasNoAirport | 1 | | record | error | PassengerHasNoFlight | 1 | @@ -177,5 +177,5 @@ Feature: Pipeline tests using the flights dataset | parameter | value | | record_count | 1 | | number_submission_rejections | 0 | - | number_record_rejections | 9 | + | number_record_rejections | 10 | | number_warnings | 0 | diff --git a/tests/testdata/flights/multi_node_file_rejection.xml b/tests/testdata/flights/multi_node_file_rejection.xml index 0958446..466a35b 100644 --- a/tests/testdata/flights/multi_node_file_rejection.xml +++ b/tests/testdata/flights/multi_node_file_rejection.xml @@ -29,6 +29,11 @@ Alice Manager + + 1 + Alice + Manager + From d1f037cbe025579d0bec047398af910e4706a7b6 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:40:11 +0100 Subject: [PATCH 10/11] test: add full regression for flights dataset to test many scenarios in a single file --- tests/features/flights.feature | 35 ++++ tests/testdata/flights/flights.dischema.json | 5 +- .../flights_data_contract_error_details.json | 6 + .../flights/flights_full_regression.xml | 173 ++++++++++++++++++ 4 files changed, 218 insertions(+), 1 deletion(-) create mode 100644 tests/testdata/flights/flights_full_regression.xml diff --git a/tests/features/flights.feature b/tests/features/flights.feature index bc295e3..e57ba52 100644 --- a/tests/features/flights.feature +++ b/tests/features/flights.feature @@ -179,3 +179,38 @@ Feature: Pipeline tests using the flights dataset | number_submission_rejections | 0 | | number_record_rejections | 10 | | number_warnings | 0 | + + Scenario: A flights submission with a rejection on a node with two mandatory nodes + Given I submit the flights file flights_full_regression.xml for processing + And A duckdb pipeline is configured with schema file 'flights.dischema.json' + And I add initial audit entries for the submission + Then the latest audit record for the submission is marked with processing status file_transformation + When I run the file transformation phase + Then the country entity is stored as a parquet after the file_transformation phase + And the airport entity is stored as a parquet after the file_transformation phase + And the flights entity is stored as a parquet after the file_transformation phase + And the passengers entity is stored as a parquet after the file_transformation phase + And the passengers entity is stored as a parquet after the file_transformation phase + And the latest audit record for the submission is marked with processing status data_contract + When I run the data contract phase + Then there are errors with the following details and associated error_count from the data_contract phase + | FailureType | ErrorCode | error_count | + | record | AirportIdIsMissing | 1 | + When I run the business rules phase + Then there are errors with the following details and associated error_count from the business_rules phase + | ErrorType | Status | ErrorCode | error_count | + | record | error | InvalidFlightDestination | 1 | + | record | error | PassengerNameMissing | 1 | + | record | error | StaffIDMissing | 4 | + | record | error | PassengerHasNoFlight | 3 | + | record | error | StaffHasNoAirport | 1 | + | record | error | FlightHasNoAirport | 2 | + | record | error | AirportHasNoStaff | 1 | + When I run the error report phase + Then An error report is produced + And The statistics entry for the submission shows the following information + | parameter | value | + | record_count | 1 | + | number_submission_rejections | 0 | + | number_record_rejections | 14 | + | number_warnings | 0 | diff --git a/tests/testdata/flights/flights.dischema.json b/tests/testdata/flights/flights.dischema.json index c862469..1ea7837 100644 --- a/tests/testdata/flights/flights.dischema.json +++ b/tests/testdata/flights/flights.dischema.json @@ -47,7 +47,10 @@ } } }, - "key_field": "airport_id" + "key_field": "airport_id", + "mandatory_fields": [ + "airport_id" + ] }, "staff": { "fields": { diff --git a/tests/testdata/flights/flights_data_contract_error_details.json b/tests/testdata/flights/flights_data_contract_error_details.json index cb6541e..78e694d 100644 --- a/tests/testdata/flights/flights_data_contract_error_details.json +++ b/tests/testdata/flights/flights_data_contract_error_details.json @@ -4,5 +4,11 @@ "error_code": "CountryIdIsMissing", "error_message": "Record Rejected - Country is missing an id" } + }, + "airport_id": { + "Blank": { + "error_code": "AirportIdIsMissing", + "error_message": "Record Rejected - Airport is missing an id" + } } } \ No newline at end of file diff --git a/tests/testdata/flights/flights_full_regression.xml b/tests/testdata/flights/flights_full_regression.xml new file mode 100644 index 0000000..34a64bd --- /dev/null +++ b/tests/testdata/flights/flights_full_regression.xml @@ -0,0 +1,173 @@ + + + 1 + England + + + 1 + 1 + Heathrow + TW6 1EW + + + 1 + 1 + Paris + + + 1 + 1 + John + + + + + + + 1 + 1 + Marge + Manager + + + + + 1 + 2 + Manchester + M90 1QX + + + 2 + 2 + Venus + + + 2 + 2 + Jane + + + + + 2 + 3 + Rome + + + 3 + 3 + + + + + + + 2 + 2 + Thomas + Pilot + + + + + 1 + Birmingham + B26 3QJ + + + 3 + 4 + Amsterdam + + + 4 + 4 + Billy + + + + + + + 3 + 3 + Joanne + Security + + + + + 1 + 4 + Leeds & Bradford + LS19 7TU + + + 4 + 5 + Dubai + + + 5 + 5 + Terry + + + + + + + 4 + Rebecca + Ground Crew + + + 4 + Tim + Ground Crew + + + 4 + Julie + Pilot + + + + + 1 + 5 + Newcastle + NE13 8BZ + + + 5 + 6 + Toronto + + + 6 + 6 + Bob + + + + + + + 5 + 8 + Jasmine + Pilot + + + 5 + Rodger + Security + + + + + + + \ No newline at end of file From 38d6a88dc0cb01a84017c3be384ed1f304c61256 Mon Sep 17 00:00:00 2001 From: georgeRobertson <50412379+georgeRobertson@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:47:34 +0100 Subject: [PATCH 11/11] test: fix flights regression test --- tests/features/flights.feature | 2 +- tests/testdata/flights/flights_full_regression.xml | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/features/flights.feature b/tests/features/flights.feature index e57ba52..14572d4 100644 --- a/tests/features/flights.feature +++ b/tests/features/flights.feature @@ -180,7 +180,7 @@ Feature: Pipeline tests using the flights dataset | number_record_rejections | 10 | | number_warnings | 0 | - Scenario: A flights submission with a rejection on a node with two mandatory nodes + Scenario: A flights submission with many types of rejections in a single submission Given I submit the flights file flights_full_regression.xml for processing And A duckdb pipeline is configured with schema file 'flights.dischema.json' And I add initial audit entries for the submission diff --git a/tests/testdata/flights/flights_full_regression.xml b/tests/testdata/flights/flights_full_regression.xml index 34a64bd..1ed73cf 100644 --- a/tests/testdata/flights/flights_full_regression.xml +++ b/tests/testdata/flights/flights_full_regression.xml @@ -168,6 +168,4 @@ - - \ No newline at end of file