Improve SDMX-VTL mapping and add data_structures/datapoints to run_sdmx#875
Improve SDMX-VTL mapping and add data_structures/datapoints to run_sdmx#875albertohernandez1995 wants to merge 9 commits into
Conversation
Covered duplicate-alias and duplicate-structure conflicts
javihern98
left a comment
javihern98
left a comment
There was a problem hiding this comment.
Code review
This review was generated by Claude Code. It surfaces 10 potential issues found while reviewing the one-to-many SDMX mapping and run_sdmx merge logic, each posted as a separate inline comment with a confidence score (0-100).
Only the first (silent data loss when one dataflow feeds multiple datasets via distinct PandasDataset objects) was independently reproduced and verified as a real, high-confidence bug. The remaining nine are lower-confidence signals included for completeness at the requester's request — treat anything below ~80 as worth a look rather than confirmed, and the two at 25-50 as likely non-issues on closer inspection.
🤖 Generated with Claude Code
- If this code review was useful, please react with 👍. Otherwise, react with 👎.
| normalized = _normalize_mappings(mappings) or {} | ||
|
|
||
| if isinstance(mappings, VtlDataflowMapping): | ||
| return _convert_vtl_dataflow_mapping(mappings) | ||
| # A VTL dataset name may come from exactly one dataflow; duplicates across (or within) | ||
| # dataflows would overwrite data silently. | ||
| all_names = [name for names in normalized.values() for name in names] | ||
| duplicates = sorted({name for name in all_names if all_names.count(name) > 1}) | ||
| if duplicates: | ||
| raise InputValidationException(code="0-1-3-11", names=duplicates) | ||
|
|
||
| return normalized |
There was a problem hiding this comment.
Silent data loss when one dataflow feeds multiple datasets (confidence: high — reproduced)
mapping_dict is keyed only by short_urn, with no check that multiple entries in datasets share a short_urn. In run_sdmx's fan-out loop (
vtlengine/src/vtlengine/API/__init__.py
Lines 622 to 628 in 6bf7598
datapoints_dict[dataset_name] = dataset.data runs on every outer iteration, so two PandasDatasets sharing a short_urn (e.g. two reporting periods of the same dataflow — called out as "a common SDMX situation" in docs/sdmx_inputs.rst) silently overwrite each other's data for every aliased name. No exception, no warning.
Reproduced directly: two datasets sharing a urn, one real and one forced to 999.0, mapped to ["DS_1", "DS_2"] — both come back 999.0. This is also the exact scenario issue #874's "Alternatives Considered" section flags as needing per-alias isolation, which this implementation doesn't achieve.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Good catch — this was a real silent overwrite. Two PandasDatasets resolving to the same short_urn now raise 0-1-3-12 in the fan-out loop instead of clobbering each other's data. Added a test with two datasets of the same dataflow.
| """ | ||
| if mappings is None: | ||
| normalized = _normalize_mappings(mappings) | ||
| if normalized is None: | ||
| return None | ||
| if isinstance(mappings, dict): | ||
| return mappings | ||
| if isinstance(mappings, VtlDataflowMapping): | ||
| return _convert_vtl_dataflow_mapping(mappings) | ||
| raise InputValidationException("Expected dict or VtlDataflowMapping type for mappings.") | ||
| return {urn: names[0] for urn, names in normalized.items()} | ||
|
|
There was a problem hiding this comment.
One-to-many sdmx_mappings silently truncated in run()/semantic_analysis() (confidence: 75/100)
This keeps only names[0] per urn (used by run()/semantic_analysis(), not run_sdmx's own fan-out), silently dropping the rest — even though the public type hint on both functions (
vtlengine/src/vtlengine/API/__init__.py
Line 181 in 6bf7598
vtlengine/src/vtlengine/API/__init__.py
Line 280 in 6bf7598
run() gets a confusing "Dataset not found" error instead of a clear signal that only the first alias was used.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Agreed. _convert_sdmx_mappings now raises 0-1-3-13 when a single structure is mapped to more than one name, so run()/semantic_analysis() fail loudly instead of dropping the extra aliases. run_sdmx forwards no mapping (its inputs are already resolved to named VTL JSON), and both docstrings now state the one-name-per-structure rule and point to run_sdmx for one-to-many.
| def _structure_dataset_names(structure: DataStructureItem) -> List[str]: | ||
| """Return the dataset/scalar names declared in a VTL JSON structure dict. | ||
|
|
||
| Only VTL JSON dicts expose their names cheaply; for other forms (Path, URL or | ||
| pysdmx objects) an empty list is returned and no collision check is performed. | ||
| """ | ||
| if not isinstance(structure, dict): | ||
| return [] | ||
| names = [ds["name"] for ds in structure.get("datasets", []) if "name" in ds] | ||
| names += [sc["name"] for sc in structure.get("scalars", []) if "name" in sc] | ||
| return names |
There was a problem hiding this comment.
Duplicate-name check bypassed for non-dict data_structures (confidence: 75/100)
This returns [] for any non-dict data_structures item (Path, str, Schema, DataStructureDefinition, Dataflow), so _merge_sdmx_data_structures's new duplicate check (0-1-3-9) never sees a name to compare for those forms, and a colliding name silently overwrites downstream via {**ds_structures, **ds}. Issue #874 (implemented by this PR) explicitly asks to "raise InputValidationException on duplicate dataset names rather than silently overriding" — this only holds for dict-form extras.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Deliberately keeping the name check to dict-form extras here, for two reasons:
- The realistic collision is already caught. A duplicate dataset needs both a structure and data. Its datapoints are keyed by name, so a
Path/JSON structure colliding with an SDMX-mapped name (e.g. bothDS_1) trips the datapoint-level0-1-3-9check. A structure-only collision with no matching datapoints is a broken input that fails downstream regardless. - Consistency with
run().run()itself merges itsdata_structureslist last-wins and never raises on duplicates for any form. IntrospectingPath/str/Schema/DSD/Dataflowhere would mean file I/O + mapping resolution inside a merge helper, and would makerun_sdmxstricter thanrun()for identical inputs.
The docstring documents that only dict-form extras are name-checked. Happy to file a follow-up if we want full-coverage duplicate detection unified across run() and run_sdmx — it'd belong post-load, on resolved names, not in the merge helper.
| return None | ||
| if isinstance(mappings, dict): | ||
| return { | ||
| urn: [names] if isinstance(names, str) else list(names) | ||
| for urn, names in mappings.items() | ||
| } |
There was a problem hiding this comment.
IndexError instead of InputValidationException on empty mapping list (confidence: 75/100)
No check that list(names) is non-empty here. _convert_sdmx_mappings a few lines below then unconditionally does names[0], so sdmx_mappings={"urn": []} — allowed by the widened public type hint Dict[str, Union[str, List[str]]] — raises an unhandled IndexError instead of the library's normal InputValidationException pattern.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Fixed — an empty name list now raises 0-1-3-14 in _normalize_mappings, before _convert_sdmx_mappings ever does names[0].
| raise InputValidationException(code="0-1-3-2", schema=schema) | ||
| if schema.short_urn not in mapping_dict: | ||
| raise InputValidationException(code="0-1-3-4", short_urn=schema.short_urn) | ||
| dataset_name = mapping_dict[schema.short_urn] | ||
| vtl_structure = to_vtl_json(schema, dataset_name) | ||
| data_structures_list.append(vtl_structure) | ||
| datapoints_dict[dataset_name] = dataset.data | ||
|
|
||
| # Validate all script inputs are mapped | ||
| missing = [name for name in input_names if name not in mapping_dict.values()] | ||
| if missing: | ||
| raise InputValidationException(code="0-1-3-6", missing=missing) | ||
| for dataset_name in mapping_dict[schema.short_urn]: | ||
| data_structures_list.append(to_vtl_json(schema, dataset_name)) | ||
| datapoints_dict[dataset_name] = dataset.data | ||
|
|
There was a problem hiding this comment.
Empty mapping list silently drops a whole dataset (confidence: 75/100)
if schema.short_urn not in mapping_dict (L623) only checks the key exists, not that its value list is non-empty. mappings={short_urn: []} for a dataflow that IS present in datasets passes this check, then the for dataset_name in mapping_dict[schema.short_urn]: loop runs zero times — the dataset's structure/data vanish with no error. Not covered by any test in tests/API/test_sdmx.py.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Same root cause as the empty-list case. Rejecting empty lists at _normalize_mappings (0-1-3-14) also prevents the zero-iteration silent drop here. Added a test.
| "0-1-3-9": { | ||
| "message": "Duplicate dataset name(s) {names} provided both by SDMX datasets and " | ||
| "explicit inputs.", | ||
| "description": "Raised when an explicit data structure or datapoints entry uses a " | ||
| "dataset name that is already provided by one of the SDMX datasets.", | ||
| }, |
There was a problem hiding this comment.
Message text misattributes the collision source (confidence: 65/100)
This says duplicates are "provided both by SDMX datasets and explicit inputs," but the check in _merge_sdmx_data_structures (
vtlengine/src/vtlengine/API/_sdmx_utils.py
Lines 232 to 239 in 6bf7598
extra side (no SDMX involvement at all) still raise this same message.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Reworded to "Duplicate dataset name(s) {names} in the combined SDMX and explicit inputs" — accurate whether the clash is SDMX-vs-explicit or between two explicit entries.
| "0-1-3-11": { | ||
| "message": "VTL dataset name(s) {names} are mapped from more than one SDMX dataflow.", | ||
| "description": "Raised when the mappings assign the same VTL dataset name to more than " | ||
| "one SDMX dataflow, which would overwrite data silently.", | ||
| }, |
There was a problem hiding this comment.
Message says "more than one dataflow" but also fires within one dataflow (confidence: 65/100)
The code comment in _build_mapping_dict (
vtlengine/src/vtlengine/API/_sdmx_utils.py
Lines 183 to 190 in 6bf7598
🤖 Generated by Claude Code
There was a problem hiding this comment.
Reworded to "VTL dataset name(s) {names} are mapped more than once" so it also fits a repeated alias within a single dataflow's list.
| data_structures: Optional extra data structures merged with the SDMX-derived ones, \ | ||
| using the same forms accepted by :obj:`run <vtlengine.API>`. (default: None) | ||
|
|
||
| datapoints: Optional extra datapoints merged with the SDMX-derived ones, using the \ | ||
| same forms accepted by :obj:`run <vtlengine.API>`. Must be a dict keyed by dataset \ | ||
| name when SDMX datasets are also provided. (default: None) | ||
|
|
||
| value_domains: Dict or Path, or List of Dicts or Paths of the \ | ||
| value domains JSON files. (default:None) It is passed as an object, that can be read from \ |
There was a problem hiding this comment.
Raises: section not updated alongside this docstring edit (confidence: 50/100)
The Args: section was extended here with data_structures/datapoints, but the Raises: section further down (L590-592, not touched by this diff) still only mentions SemanticError and doesn't list the three new codes (0-1-3-9, 0-1-3-10, 0-1-3-11) this function now raises.
🤖 Generated by Claude Code
There was a problem hiding this comment.
Updated the Raises: section to list InputValidationException with the new codes (0-1-3-9/10/11/12/14).
Two PandasDatasets with the same short-URN silently overwrote each other's datapoints in run_sdmx's fan-out loop. Raise 0-1-3-12 instead.
Those functions map each SDMX structure to a single dataset; a multi-name mapping now raises 0-1-3-13 instead of silently keeping the first alias. run_sdmx forwards no mapping since its inputs are already resolved.
An empty list ({"urn": []}) raised IndexError / silently dropped a dataset;
raise 0-1-3-14 with a clear message instead.
0-1-3-9 no longer misattributes the collision source; 0-1-3-11 covers repeats within one dataflow; run_sdmx's Raises section lists the new codes.
2 participants
Summary
run_sdmxhad two rough edges when wiring SDMX inputs into a VTL script:referenced the same source under two names, there was no way to express it — the
internal mapping was keyed by short-URN, so the second name was simply lost.
run,run_sdmxcouldn't take extra inputs. If one dataset came from SDMXand another from a local CSV, you had to drop down to
runand hand-build theVTL JSON yourself.
This PR fixes both.
Mapping (one dataflow → many datasets).
mappingsnow accepts, on top of theexisting dict and single
VtlDataflowMapping:{"Dataflow=MD:DF(1.0)": ["DS_1", "DS_2"]}VtlDataflowMapping(same dataflow, differentdataflow_alias)VtlMappingSchemeInternally the mapping became
{short_urn: [names...]}and the conversion loop fansout one structure + datapoints copy per alias. The widened type also applies to
run/semantic_analysis'ssdmx_mappings.Explicit inputs.
run_sdmxgained optionaldata_structuresanddatapointsparams mirroring
run. They're merged with the SDMX-derived inputs before execution,so you can mix an SDMX dataflow with a plain VTL JSON structure + DataFrame/CSV in one
call.
Guardrails. Conflicts that used to overwrite data silently now raise:
0-1-3-9— a name is provided by both an SDMX dataset and an explicit input0-1-3-10— non-dict datapoints combined with SDMX datasets (no name to merge on)0-1-3-11— the same VTL name is mapped from more than one dataflowChecklist
ruff format,ruff check,mypy)pytest— full suite green)worked example is a possible follow-up
Impact / Risk
run_sdmxparams are optional and default to theprevious behaviour; the
mappings/sdmx_mappingstypes are only widened, so existingdict /
VtlDataflowMappingcalls are unaffected.run/semantic_analysisstructure-loadingpath, a many-name mapping still resolves one SDMX structure to a single name (first
wins); the one-to-many fan-out is specific to
run_sdmx.run_sdmx; threenew validation error codes (
0-1-3-9/10/11).Notes
data/vtl/, inputs from files, andresults compared against committed reference outputs (codes
3-1,3-2).run/semantic_analysisfor SDMX structure objects, honouring
ToVtlMapping/FromVtlMappingsub-space filters,and a docs example.
Closes #874