From 5fcd3de28e7f318fffb31b9e8eabe91dde45a1ef Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Mon, 16 Mar 2026 19:31:30 -0400 Subject: [PATCH 1/7] error handling and removed if conditional --- .../utilities/data_processor.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/cdisc_rules_engine/utilities/data_processor.py b/cdisc_rules_engine/utilities/data_processor.py index 04d6dbe46..9e4f7c3ee 100644 --- a/cdisc_rules_engine/utilities/data_processor.py +++ b/cdisc_rules_engine/utilities/data_processor.py @@ -17,6 +17,7 @@ DataServiceFactory, DummyDataService, ) +from cdisc_rules_engine.exceptions.custom_exceptions import PreprocessingError from cdisc_rules_engine.utilities.utils import ( search_in_list_of_dicts, ) @@ -211,6 +212,9 @@ def merge_pivot_supp_dataset( for key in static_keys if key in left_dataset.columns and key in right_dataset.columns ] + DataProcessor._validate_merge_key_overlap( + left_dataset, right_dataset, common_keys + ) if not is_blank: common_keys.append(dynamic_key) current_supp = right_dataset.rename(columns={"IDVARVAL": dynamic_key}) @@ -287,6 +291,9 @@ def _merge_supp_with_multiple_idvars( for key in static_keys if key in result_dataset.columns and key in group_data.columns ] + DataProcessor._validate_merge_key_overlap( + result_dataset, group_data, common_keys + ) common_keys.append(idvar_value) agg_dict = { @@ -480,3 +487,19 @@ def column_metadata_equal_to_define_and_library( @staticmethod def is_dummy_data(data_service: DataServiceInterface) -> bool: return isinstance(data_service, DummyDataService) + + @staticmethod + def _validate_merge_key_overlap( + left_dataset: DatasetInterface, + right_dataset: DatasetInterface, + common_keys: List[str], + ): + for key in common_keys: + left_values = set(left_dataset[key].dropna().unique()) + right_values = set(right_dataset[key].dropna().unique()) + if left_values and right_values and left_values.isdisjoint(right_values): + raise PreprocessingError( + f"SUPP merge key '{key}' has no overlapping values between " + f"parent dataset and SUPP dataset. " + f"Parent values: {left_values}, SUPP values: {right_values}." + ) From 7772a526427f80a746cfa6a49512512ba7fa763c Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Mon, 16 Mar 2026 19:45:29 -0400 Subject: [PATCH 2/7] added test --- tests/unit/test_dataset_preprocessor.py | 65 ++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_dataset_preprocessor.py b/tests/unit/test_dataset_preprocessor.py index 2b238ce9d..65fe8b519 100644 --- a/tests/unit/test_dataset_preprocessor.py +++ b/tests/unit/test_dataset_preprocessor.py @@ -17,7 +17,7 @@ from cdisc_rules_engine.models.library_metadata_container import ( LibraryMetadataContainer, ) - +from cdisc_rules_engine.exceptions.custom_exceptions import PreprocessingError from cdisc_rules_engine.models.dataset import PandasDataset @@ -1291,6 +1291,69 @@ def test_dm_merged_with_suppdm_without_dupes( assert result.data.loc[0, ["RACE1", "RACE2", "RACE3"]].notna().all() +@patch("cdisc_rules_engine.services.data_services.LocalDataService.get_dataset") +def test_preprocess_suppae_mismatched_studyid_raises_key_error(mock_get_dataset): + ae_dataset = PandasDataset( + pd.DataFrame( + { + "STUDYID": ["CDISC-PILOT-01"], + "DOMAIN": ["AE"], + "USUBJID": ["S001"], + "AESEQ": [1], + "AETERM": ["Headache"], + } + ) + ) + suppae_dataset = PandasDataset( + pd.DataFrame( + { + "STUDYID": ["COMPLETELY-DIFFERENT-STUDY"], + "RDOMAIN": ["AE"], + "USUBJID": ["S001"], + "IDVAR": ["AESEQ"], + "IDVARVAL": ["1"], + "QNAM": ["TEST"], + "QLABEL": ["Test"], + "QVAL": ["A"], + } + ) + ) + + mock_get_dataset.return_value = suppae_dataset + rule = { + "core_id": "TestRule", + "datasets": [{"domain_name": "SUPPAE", "match_key": ["USUBJID"]}], + "conditions": ConditionCompositeFactory.get_condition_composite( + { + "all": [ + { + "name": "get_dataset", + "operator": "equal_to", + "value": {"target": "TEST", "comparator": "A"}, + } + ] + } + ), + } + datasets = [ + SDTMDatasetMetadata( + name="SUPPAE", first_record={"RDOMAIN": "AE"}, filename="suppae.xpt" + ) + ] + data_service = LocalDataService(MagicMock(), MagicMock(), MagicMock()) + preprocessor = DatasetPreprocessor( + ae_dataset, + SDTMDatasetMetadata(first_record={"DOMAIN": "AE"}, full_path="path"), + data_service, + InMemoryCacheService(), + ) + + with pytest.raises( + PreprocessingError, match="SUPP merge key 'STUDYID' has no overlapping values" + ): + preprocessor.preprocess(rule, datasets) + + def test_relrec_processed_correctly_with_others(rule_with_specific_supp): ec_meta = SDTMDatasetMetadata( name="EC", From 9af5403c14239380a854fbd43185d82a9a0c4d64 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Wed, 18 Mar 2026 09:36:49 -0400 Subject: [PATCH 3/7] remove if --- cdisc_rules_engine/utilities/data_processor.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/cdisc_rules_engine/utilities/data_processor.py b/cdisc_rules_engine/utilities/data_processor.py index 9e4f7c3ee..7e217c787 100644 --- a/cdisc_rules_engine/utilities/data_processor.py +++ b/cdisc_rules_engine/utilities/data_processor.py @@ -358,14 +358,13 @@ def process_supp(supp_dataset): columns_to_drop = [ col for col in ["QNAM", "QVAL", "QLABEL"] if col in supp_dataset.columns ] - if "RDOMAIN" in supp_dataset.columns and supp_dataset["RDOMAIN"][0] == "DM": - excluded_columns = list(supp_dataset["QNAM"].unique()) + columns_to_drop - group_cols = [c for c in supp_dataset.columns if c not in excluded_columns] - supp_dataset = PandasDataset( - supp_dataset.data.groupby(group_cols, dropna=False, as_index=False).agg( - lambda x: (x.dropna().iloc[0] if not x.dropna().empty else pd.NA) - ) + excluded_columns = list(supp_dataset["QNAM"].unique()) + columns_to_drop + group_cols = [c for c in supp_dataset.columns if c not in excluded_columns] + supp_dataset = PandasDataset( + supp_dataset.data.groupby(group_cols, dropna=False, as_index=False).agg( + lambda x: (x.dropna().iloc[0] if not x.dropna().empty else pd.NA) ) + ) if columns_to_drop: supp_dataset = supp_dataset.drop(labels=columns_to_drop, axis=1) return supp_dataset From c539cfa110d6fd0827f0bb9901bec70c933f2cc2 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Wed, 18 Mar 2026 10:05:47 -0400 Subject: [PATCH 4/7] test --- cdisc_rules_engine/utilities/data_processor.py | 13 ++++++++++--- tests/unit/test_merge_supp_datasets.py | 5 +++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/cdisc_rules_engine/utilities/data_processor.py b/cdisc_rules_engine/utilities/data_processor.py index 7e217c787..912343097 100644 --- a/cdisc_rules_engine/utilities/data_processor.py +++ b/cdisc_rules_engine/utilities/data_processor.py @@ -338,7 +338,7 @@ def _merge_supp_with_multiple_idvars( validation_keys.append(idvar_for_qnam) grouped = qnam_check.groupby(validation_keys).size() if (grouped > 1).any(): - raise ValueError( + raise PreprocessingError( f"Multiple records with the same QNAM '{qnam}' match a single parent record" ) return result_dataset @@ -360,6 +360,13 @@ def process_supp(supp_dataset): ] excluded_columns = list(supp_dataset["QNAM"].unique()) + columns_to_drop group_cols = [c for c in supp_dataset.columns if c not in excluded_columns] + grouped = supp_dataset.data.groupby( + group_cols, dropna=False, as_index=False + ).size() + if (grouped["size"] > 1).any(): + raise PreprocessingError( + "Multiple records with the same QNAM match a single parent record" + ) supp_dataset = PandasDataset( supp_dataset.data.groupby(group_cols, dropna=False, as_index=False).agg( lambda x: (x.dropna().iloc[0] if not x.dropna().empty else pd.NA) @@ -383,7 +390,7 @@ def _validate_qnam( continue grouped = qnam_check.groupby(common_keys).size() if (grouped > 1).any(): - raise ValueError( + raise PreprocessingError( f"Multiple records with the same QNAM '{qnam}' match a single parent record" ) @@ -403,7 +410,7 @@ def _validate_qnam_dask( problem_groups = grouped_counts[grouped_counts > 1] problem_groups_computed = problem_groups.compute() if len(problem_groups_computed) > 0: - raise ValueError( + raise PreprocessingError( f"Multiple records with the same QNAM '{qnam}' match a single parent record. " ) diff --git a/tests/unit/test_merge_supp_datasets.py b/tests/unit/test_merge_supp_datasets.py index 370bdb728..955d6e5e8 100644 --- a/tests/unit/test_merge_supp_datasets.py +++ b/tests/unit/test_merge_supp_datasets.py @@ -7,6 +7,7 @@ from cdisc_rules_engine.utilities.data_processor import DataProcessor import pandas as pd import pandas.testing as pdt +from cdisc_rules_engine.exceptions.custom_exceptions import PreprocessingError @pytest.fixture @@ -258,7 +259,7 @@ def test_merge_supp_dataset_multi_idvar_aggregation( @patch.object(LocalDataService, "check_filepath", return_value=False) @patch.object(LocalDataService, "_async_get_datasets") -def test_merge_supp_dataset_multi_idvar_same_qnam_validation_error( +def test_merge_supp_dataset_same_qnam_validation_error( mock_async_get_datasets, data_service ): parent_dataset = PandasDataset( @@ -292,7 +293,7 @@ def test_merge_supp_dataset_multi_idvar_same_qnam_validation_error( mock_async_get_datasets.return_value = [parent_dataset, supp_dataset] - with pytest.raises(ValueError, match="Multiple records with the same QNAM"): + with pytest.raises(PreprocessingError, match="Multiple records with the same QNAM"): DataProcessor.merge_pivot_supp_dataset( data_service.dataset_implementation, parent_dataset, supp_dataset ) From 8039f5f079ae7a5adfe76893a19b213206490e1e Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Wed, 18 Mar 2026 10:15:54 -0400 Subject: [PATCH 5/7] same QNAM --- cdisc_rules_engine/utilities/data_processor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cdisc_rules_engine/utilities/data_processor.py b/cdisc_rules_engine/utilities/data_processor.py index 912343097..4f780acb3 100644 --- a/cdisc_rules_engine/utilities/data_processor.py +++ b/cdisc_rules_engine/utilities/data_processor.py @@ -361,7 +361,7 @@ def process_supp(supp_dataset): excluded_columns = list(supp_dataset["QNAM"].unique()) + columns_to_drop group_cols = [c for c in supp_dataset.columns if c not in excluded_columns] grouped = supp_dataset.data.groupby( - group_cols, dropna=False, as_index=False + group_cols + ["QNAM"], dropna=False, as_index=False ).size() if (grouped["size"] > 1).any(): raise PreprocessingError( From dc30ab0a6582749d24385bf380b396a4f84ccdcd Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Wed, 18 Mar 2026 10:27:15 -0400 Subject: [PATCH 6/7] unit test --- .../utilities/data_processor.py | 22 ----------------- tests/unit/test_merge_supp_datasets.py | 24 +++++++++++++++++++ 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/cdisc_rules_engine/utilities/data_processor.py b/cdisc_rules_engine/utilities/data_processor.py index 4f780acb3..82c9d6c22 100644 --- a/cdisc_rules_engine/utilities/data_processor.py +++ b/cdisc_rules_engine/utilities/data_processor.py @@ -212,9 +212,6 @@ def merge_pivot_supp_dataset( for key in static_keys if key in left_dataset.columns and key in right_dataset.columns ] - DataProcessor._validate_merge_key_overlap( - left_dataset, right_dataset, common_keys - ) if not is_blank: common_keys.append(dynamic_key) current_supp = right_dataset.rename(columns={"IDVARVAL": dynamic_key}) @@ -291,9 +288,6 @@ def _merge_supp_with_multiple_idvars( for key in static_keys if key in result_dataset.columns and key in group_data.columns ] - DataProcessor._validate_merge_key_overlap( - result_dataset, group_data, common_keys - ) common_keys.append(idvar_value) agg_dict = { @@ -493,19 +487,3 @@ def column_metadata_equal_to_define_and_library( @staticmethod def is_dummy_data(data_service: DataServiceInterface) -> bool: return isinstance(data_service, DummyDataService) - - @staticmethod - def _validate_merge_key_overlap( - left_dataset: DatasetInterface, - right_dataset: DatasetInterface, - common_keys: List[str], - ): - for key in common_keys: - left_values = set(left_dataset[key].dropna().unique()) - right_values = set(right_dataset[key].dropna().unique()) - if left_values and right_values and left_values.isdisjoint(right_values): - raise PreprocessingError( - f"SUPP merge key '{key}' has no overlapping values between " - f"parent dataset and SUPP dataset. " - f"Parent values: {left_values}, SUPP values: {right_values}." - ) diff --git a/tests/unit/test_merge_supp_datasets.py b/tests/unit/test_merge_supp_datasets.py index 955d6e5e8..55f438928 100644 --- a/tests/unit/test_merge_supp_datasets.py +++ b/tests/unit/test_merge_supp_datasets.py @@ -56,6 +56,30 @@ def test_process_supp(): assert "QLABEL" not in processed_dataset.data.columns, "'QVAL' should be dropped." +def test_data_processor_suppae_multiple_qnams(): + suppae_data = { + "STUDYID": ["CDISCPILOT01", "CDISCPILOT01"], + "RDOMAIN": ["AE", "AE"], + "USUBJID": ["CDISC008", "CDISC008"], + "IDVAR": ["", ""], + "IDVARVAL": ["", ""], + "QNAM": ["AESPID", "AEREL2"], + "QLABEL": ["Sponsor ID", "Relationship 2"], + "QVAL": ["SP001", "POSSIBLE"], + "QORIG": ["CRF", "CRF"], + "QEVAL": ["", ""], + } + suppae_ds = PandasDataset(pd.DataFrame(suppae_data)) + assert suppae_ds.data.shape[0] == 2 + + result = DataProcessor().process_supp(suppae_ds).data + + assert result.shape[0] == 1 + assert {"AESPID", "AEREL2"}.issubset(set(result.columns)) + assert result.loc[0, "AESPID"] == "SP001" + assert result.loc[0, "AEREL2"] == "POSSIBLE" + + @patch.object(LocalDataService, "check_filepath", return_value=False) @patch.object(LocalDataService, "_async_get_datasets") def test_merge_pivot_supp_dataset( From efb9aa42d943950f4bcc9d46206d2047b7d77f3d Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 19 Mar 2026 09:50:50 -0400 Subject: [PATCH 7/7] reverted test --- tests/unit/test_dataset_preprocessor.py | 64 ------------------------- 1 file changed, 64 deletions(-) diff --git a/tests/unit/test_dataset_preprocessor.py b/tests/unit/test_dataset_preprocessor.py index 65fe8b519..de6a1ce57 100644 --- a/tests/unit/test_dataset_preprocessor.py +++ b/tests/unit/test_dataset_preprocessor.py @@ -17,7 +17,6 @@ from cdisc_rules_engine.models.library_metadata_container import ( LibraryMetadataContainer, ) -from cdisc_rules_engine.exceptions.custom_exceptions import PreprocessingError from cdisc_rules_engine.models.dataset import PandasDataset @@ -1291,69 +1290,6 @@ def test_dm_merged_with_suppdm_without_dupes( assert result.data.loc[0, ["RACE1", "RACE2", "RACE3"]].notna().all() -@patch("cdisc_rules_engine.services.data_services.LocalDataService.get_dataset") -def test_preprocess_suppae_mismatched_studyid_raises_key_error(mock_get_dataset): - ae_dataset = PandasDataset( - pd.DataFrame( - { - "STUDYID": ["CDISC-PILOT-01"], - "DOMAIN": ["AE"], - "USUBJID": ["S001"], - "AESEQ": [1], - "AETERM": ["Headache"], - } - ) - ) - suppae_dataset = PandasDataset( - pd.DataFrame( - { - "STUDYID": ["COMPLETELY-DIFFERENT-STUDY"], - "RDOMAIN": ["AE"], - "USUBJID": ["S001"], - "IDVAR": ["AESEQ"], - "IDVARVAL": ["1"], - "QNAM": ["TEST"], - "QLABEL": ["Test"], - "QVAL": ["A"], - } - ) - ) - - mock_get_dataset.return_value = suppae_dataset - rule = { - "core_id": "TestRule", - "datasets": [{"domain_name": "SUPPAE", "match_key": ["USUBJID"]}], - "conditions": ConditionCompositeFactory.get_condition_composite( - { - "all": [ - { - "name": "get_dataset", - "operator": "equal_to", - "value": {"target": "TEST", "comparator": "A"}, - } - ] - } - ), - } - datasets = [ - SDTMDatasetMetadata( - name="SUPPAE", first_record={"RDOMAIN": "AE"}, filename="suppae.xpt" - ) - ] - data_service = LocalDataService(MagicMock(), MagicMock(), MagicMock()) - preprocessor = DatasetPreprocessor( - ae_dataset, - SDTMDatasetMetadata(first_record={"DOMAIN": "AE"}, full_path="path"), - data_service, - InMemoryCacheService(), - ) - - with pytest.raises( - PreprocessingError, match="SUPP merge key 'STUDYID' has no overlapping values" - ): - preprocessor.preprocess(rule, datasets) - - def test_relrec_processed_correctly_with_others(rule_with_specific_supp): ec_meta = SDTMDatasetMetadata( name="EC",