From 05045748ff1f9f0d4b59c5e8560f94bcbc0635ce Mon Sep 17 00:00:00 2001 From: Alberto Date: Tue, 14 Jul 2026 13:08:51 +0200 Subject: [PATCH 1/2] Fix #892: materialize viral-propagation operand and unpivot window once --- .../duckdb_transpiler/Transpiler/__init__.py | 31 +++++-- tests/duckdb_transpiler/test_run.py | 90 +++++++++++++++++++ 2 files changed, 112 insertions(+), 9 deletions(-) diff --git a/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py b/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py index ad3146de9..968957c18 100644 --- a/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py +++ b/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py @@ -1997,14 +1997,20 @@ def visit_RegularAggregation_unpivot(self, node: AST.RegularAggregation) -> str: qn = quote_name(comp.name) excl_list.append(qn) expr_list.append(f"{vp_dataset_wide_sql(v_rule, qn)} AS {qn}") + cte: Optional[CTEBuilder] = None if expr_list: - table_src = ( - f"(SELECT * EXCLUDE ({', '.join(excl_list)}), {', '.join(expr_list)} " - f"FROM {table_src} AS _uv_in) AS _uv_src" + cte = CTEBuilder() + cte.cte( + "_uv_src", + f"SELECT * EXCLUDE ({', '.join(excl_list)}), {', '.join(expr_list)} " + f"FROM {table_src} AS _uv_in", + materialized=True, ) + table_src = "_uv_src" if not measure_names: - return f"SELECT * FROM {table_src}" + sql = f"SELECT * FROM {table_src}" + return cte.select(sql) if cte is not None else sql parts: List[str] = [] for measure in measure_names: @@ -2019,7 +2025,8 @@ def visit_RegularAggregation_unpivot(self, node: AST.RegularAggregation) -> str: ) parts.append(part) - return " UNION ALL ".join(parts) + union_sql = " UNION ALL ".join(parts) + return cte.select(union_sql) if cte is not None else union_sql # Aggregation visitor @@ -3337,10 +3344,16 @@ def _build_hierarchy_sql( cols = [quote_name(c) for c in ds.get_components_names()] return f"SELECT {', '.join(cols)} FROM {table_src}" + cte = CTEBuilder() + has_viral = any(c.role == Role.VIRAL_ATTRIBUTE for c in ds.components.values()) + base_src = table_src + if has_viral and table_src.lstrip().startswith("("): + cte.cte("_op", f"SELECT * FROM {table_src}", materialized=True) + base_src = "_op" + pivot_sql, measure, other_ids, unique_items = self._build_hr_pivot( - table_src, ds, parsed_rules, rule_comp, cond_mapping + base_src, ds, parsed_rules, rule_comp, cond_mapping ) - cte = CTEBuilder() # MATERIALIZED to avoid the optimizer re-inlining the pivot aggregation # into every dependent CTE in the chain below. cte.cte("_pivot", pivot_sql, materialized=True) @@ -3406,7 +3419,7 @@ def _build_hierarchy_sql( # Combine child viral values into each computed node (issue #877). vp_info = self._build_hierarchy_viral_ctes( - cte, ds, parsed_rules, rule_comp, other_ids, table_src + cte, ds, parsed_rules, rule_comp, other_ids, base_src ) if vp_info is not None: vp_name, viral_names = vp_info @@ -3431,7 +3444,7 @@ def _build_hierarchy_sql( cte.cte("_computed", computed_sql) cte.cte( "_combined", - f"SELECT {all_cols_csv}, 0 AS _src FROM {table_src} " + f"SELECT {all_cols_csv}, 0 AS _src FROM {base_src} " f"UNION ALL SELECT {all_cols_csv}, 1 AS _src FROM _computed", ) return cte.select( diff --git a/tests/duckdb_transpiler/test_run.py b/tests/duckdb_transpiler/test_run.py index 9b66baa25..f926f5ff9 100644 --- a/tests/duckdb_transpiler/test_run.py +++ b/tests/duckdb_transpiler/test_run.py @@ -3266,3 +3266,93 @@ def test_rule_priority_mode(self, hierarchy_structures): ) expected = expected.sort_values(["Id_1", "Id_2"]).reset_index(drop=True) pd.testing.assert_frame_equal(result, expected, check_dtype=False, check_like=True) + + +class TestViralPropagationSQLDeduplication: + """SQL-structure tests for viral propagation transpilation (issue #892). + + The generated SQL must not repeat expensive work: a derived hierarchy operand + is materialized once instead of re-embedded per leaf child, and the unpivot's + dataset-wide viral window is computed once instead of per measure arm. + """ + + def test_hierarchy_derived_operand_materialized_once(self): + """The derived operand appears once, referenced by both pivot and viral CTEs.""" + script = """ + define viral propagation VP (variable VAt_1) is aggregate max end viral propagation; + define hierarchical ruleset H (valuedomain rule Id_2) is + A = B + C; T = A + D; U = T + E + end hierarchical ruleset; + DS_r <- hierarchy(DS_1[filter Me_1 > 0], H rule Id_2 non_null); + """ + data_structures = { + "datasets": [ + { + "name": "DS_1", + "DataStructure": [ + { + "name": "Id_1", + "type": "Integer", + "role": "Identifier", + "nullable": False, + }, + {"name": "Id_2", "type": "String", "role": "Identifier", "nullable": False}, + {"name": "Me_1", "type": "Number", "role": "Measure", "nullable": True}, + { + "name": "VAt_1", + "type": "Number", + "role": "Viral Attribute", + "nullable": True, + }, + ], + } + ] + } + + queries = {name: sql for name, sql, _ in transpile(script, data_structures)} + + operand = 'FROM "DS_1" WHERE ("Me_1" > 0)' + assert queries["DS_r"].count(operand) == 1, ( + f"Derived operand should be materialized once, found " + f"{queries['DS_r'].count(operand)} copies:\n{queries['DS_r']}" + ) + + def test_unpivot_viral_window_computed_once(self): + """The dataset-wide viral window appears once, referenced by every measure arm.""" + script = ( + "define viral propagation VP (variable VAt_1) is aggregate max " + "end viral propagation;\n" + "DS_u <- DS_2[unpivot Id_2, Val];" + ) + data_structures = { + "datasets": [ + { + "name": "DS_2", + "DataStructure": [ + { + "name": "Id_1", + "type": "Integer", + "role": "Identifier", + "nullable": False, + }, + {"name": "Me_1", "type": "Number", "role": "Measure", "nullable": True}, + {"name": "Me_2", "type": "Number", "role": "Measure", "nullable": True}, + {"name": "Me_3", "type": "Number", "role": "Measure", "nullable": True}, + { + "name": "VAt_1", + "type": "Number", + "role": "Viral Attribute", + "nullable": True, + }, + ], + } + ] + } + + queries = {name: sql for name, sql, _ in transpile(script, data_structures)} + + window = 'MAX("VAt_1") OVER ()' + assert queries["DS_u"].count(window) == 1, ( + f"Dataset-wide viral window should be computed once, found " + f"{queries['DS_u'].count(window)} copies:\n{queries['DS_u']}" + ) From 2bf5c0e3e563afa63bc9fa11403f2921c8e9b039 Mon Sep 17 00:00:00 2001 From: Alberto Date: Wed, 15 Jul 2026 15:35:45 +0200 Subject: [PATCH 2/2] Address #892 review: drop hierarchy operand materialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Benchmarks (DS_1 1.2M / DS_2 3M rows, DuckDB threads=4) show the _op MATERIALIZED CTE regresses hierarchy ~12% by blocking filter pushdown, with no measurable win — DuckDB already plans the repeated inline operand efficiently. Revert that part; keep the unpivot _uv_src window dedup (EXPLAIN: WINDOW/SEQ_SCAN 3->1, scales with measure count). --- .../duckdb_transpiler/Transpiler/__init__.py | 14 ++---- tests/duckdb_transpiler/test_run.py | 48 ------------------- 2 files changed, 4 insertions(+), 58 deletions(-) diff --git a/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py b/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py index 968957c18..9f75a41b5 100644 --- a/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py +++ b/src/vtlengine/duckdb_transpiler/Transpiler/__init__.py @@ -3344,16 +3344,10 @@ def _build_hierarchy_sql( cols = [quote_name(c) for c in ds.get_components_names()] return f"SELECT {', '.join(cols)} FROM {table_src}" - cte = CTEBuilder() - has_viral = any(c.role == Role.VIRAL_ATTRIBUTE for c in ds.components.values()) - base_src = table_src - if has_viral and table_src.lstrip().startswith("("): - cte.cte("_op", f"SELECT * FROM {table_src}", materialized=True) - base_src = "_op" - pivot_sql, measure, other_ids, unique_items = self._build_hr_pivot( - base_src, ds, parsed_rules, rule_comp, cond_mapping + table_src, ds, parsed_rules, rule_comp, cond_mapping ) + cte = CTEBuilder() # MATERIALIZED to avoid the optimizer re-inlining the pivot aggregation # into every dependent CTE in the chain below. cte.cte("_pivot", pivot_sql, materialized=True) @@ -3419,7 +3413,7 @@ def _build_hierarchy_sql( # Combine child viral values into each computed node (issue #877). vp_info = self._build_hierarchy_viral_ctes( - cte, ds, parsed_rules, rule_comp, other_ids, base_src + cte, ds, parsed_rules, rule_comp, other_ids, table_src ) if vp_info is not None: vp_name, viral_names = vp_info @@ -3444,7 +3438,7 @@ def _build_hierarchy_sql( cte.cte("_computed", computed_sql) cte.cte( "_combined", - f"SELECT {all_cols_csv}, 0 AS _src FROM {base_src} " + f"SELECT {all_cols_csv}, 0 AS _src FROM {table_src} " f"UNION ALL SELECT {all_cols_csv}, 1 AS _src FROM _computed", ) return cte.select( diff --git a/tests/duckdb_transpiler/test_run.py b/tests/duckdb_transpiler/test_run.py index f926f5ff9..0513b2f61 100644 --- a/tests/duckdb_transpiler/test_run.py +++ b/tests/duckdb_transpiler/test_run.py @@ -3269,54 +3269,6 @@ def test_rule_priority_mode(self, hierarchy_structures): class TestViralPropagationSQLDeduplication: - """SQL-structure tests for viral propagation transpilation (issue #892). - - The generated SQL must not repeat expensive work: a derived hierarchy operand - is materialized once instead of re-embedded per leaf child, and the unpivot's - dataset-wide viral window is computed once instead of per measure arm. - """ - - def test_hierarchy_derived_operand_materialized_once(self): - """The derived operand appears once, referenced by both pivot and viral CTEs.""" - script = """ - define viral propagation VP (variable VAt_1) is aggregate max end viral propagation; - define hierarchical ruleset H (valuedomain rule Id_2) is - A = B + C; T = A + D; U = T + E - end hierarchical ruleset; - DS_r <- hierarchy(DS_1[filter Me_1 > 0], H rule Id_2 non_null); - """ - data_structures = { - "datasets": [ - { - "name": "DS_1", - "DataStructure": [ - { - "name": "Id_1", - "type": "Integer", - "role": "Identifier", - "nullable": False, - }, - {"name": "Id_2", "type": "String", "role": "Identifier", "nullable": False}, - {"name": "Me_1", "type": "Number", "role": "Measure", "nullable": True}, - { - "name": "VAt_1", - "type": "Number", - "role": "Viral Attribute", - "nullable": True, - }, - ], - } - ] - } - - queries = {name: sql for name, sql, _ in transpile(script, data_structures)} - - operand = 'FROM "DS_1" WHERE ("Me_1" > 0)' - assert queries["DS_r"].count(operand) == 1, ( - f"Derived operand should be materialized once, found " - f"{queries['DS_r'].count(operand)} copies:\n{queries['DS_r']}" - ) - def test_unpivot_viral_window_computed_once(self): """The dataset-wide viral window appears once, referenced by every measure arm.""" script = (