feat(lineage): 游标 %ROWTYPE 记录变量与目标列表标量子查询的列级血缘穿透 (fix #142) - #153
Conversation
| }); | ||
| } | ||
| } | ||
| if matches!(sources.as_slice(), [ColumnSource::Column { .. }]) { |
There was a problem hiding this comment.
[bug] push_subquery_column_mapping does not reuse classify_value_expr. It walks the first select-list expression with collect_value_sources, then marks MappingKind::Direct (and later drops expression at 3433) whenever that walk yields exactly one Column source. That violates MappingKind::Direct (“plain copy of a single column, with no transformation”). (SELECT UPPER(r.code) FROM t_ref r), (SELECT r.code + 1 …), (SELECT NVL(r.code, 'x') …), and (SELECT CAST(r.code AS varchar) …) all become a direct copy of t_ref.code with no expression text; lineage display ([direct], transform summary) and any consumer that treats Direct as an identity hop will be wrong. The same helper also leaves (SELECT 'x' FROM dual) as Derived even after synthesizing a Literal source (3409–3415), unlike classify_value_expr which marks literals Direct. The two unit tests only cover a bare column copy, so this does not fail CI.
Suggestion: After swapping alias map / scope_sole_table, call classify_value_expr(first) (or share that function’s match) instead of collect_value_sources + the ad-hoc Direct/literal branches. Add a test that (SELECT UPPER(r.code) FROM t_ref r) is Derived with the expression kept and source t_ref.code, and that (SELECT 'x' FROM dual) is Direct + Literal.
| select: &SelectStatement, | ||
| ) { | ||
| let saved_alias_map = self.alias_map.clone(); | ||
| self.collect_aliases_from_table_refs(&select.from); |
There was a problem hiding this comment.
[suggestion] Scope isolation only save/restores alias_map and scope_sole_table. collect_aliases_from_table_refs is not a pure alias collector: on AstTableRef::Join it calls process_expr_for_joins_and_filters, which appends join_conditions, hard_filters, and column_refs to the enclosing statement’s ColumnAnalysis. Those vectors are not restored. ColumnAccessExtractor also does not walk into Expr::Subquery later (walk_expr_for_column_refs explicitly skips them), so a scalar subquery such as (SELECT r.code FROM t_ref r JOIN t_other o ON r.id = o.id WHERE …) permanently attaches the inner JOIN as if it belonged to the INSERT/UPDATE. Correlated first-expr resolution still works because outer aliases are merged, but join/filter consumers of the parent analysis see leaked edges.
Suggestion: Collect inner aliases without recording joins (a thin helper that only fills alias_map), or snapshot/restore join_conditions, hard_filters, and column_refs around the subquery walk. A unit test with a JOIN inside the scalar subquery should assert the parent analysis’s join_conditions stay empty (or match the outer query only).
| // table's columns. (A custom record TYPE anchor is rare; it | ||
| // would attribute the type name as a table — the field is | ||
| // still attributable, unlike the old `?.field`.) | ||
| return ColumnSource::Column { |
There was a problem hiding this comment.
[suggestion] The table-anchor fallback attributes r.field to the %ROWTYPE type name (t_src) whenever cursor_sources has no entry for that name. That ignores the FETCH that actually filled the record. FETCH other_cur INTO r where other_cur reads t_other still yields t_src.field — silently wrong data lineage, and not listed among the PR’s known limitations. The e2e table_rowtype_record_insert_values_resolves_to_table cannot catch this because its cursor also selects from t_src. visit_pl_statement already records FETCH cur INTO r in fetch_vars, but nothing rebinds record_cursors[r] from the type name to cur.
Suggestion: On FETCH cur INTO rec, if rec is a %ROWTYPE record, rebind record_cursors[rec] = cur so the existing cursor-source path wins; keep the table-name fallback only when no FETCH (or no cursor sources) exists. Document the FETCH≠type mismatch if you intentionally keep type-origin semantics. Add a regression: r t_src%ROWTYPE + CURSOR cur IS SELECT id FROM t_other + FETCH cur INTO r + VALUES (r.id) must resolve to t_other.id, not t_src.id.
| let mut expression: Option<String> = None; | ||
| if let Some(SelectTarget::Expr(first, _)) = select.targets.first() { | ||
| let first = peel_parenthesized(first); | ||
| self.collect_value_sources(first, &mut sources); |
There was a problem hiding this comment.
[suggestion] The claimed limitation “子查询首表达式为记录字段 → FieldAccess → Variable” does not match current ogsql-parser (v0.10.0). r.field / v_fund_acnt_all.CLIENT_ACNT_ID parse as Expr::ColumnRef (dotted names with len() >= 2 are never PlVariable or FieldAccess; FieldAccess is only built from parenthesized/function/cursor-attr receivers). collect_value_sources on that ColumnRef already calls column_source, which consults record_cursors before aliases. Inside a procedure, a %ROWTYPE record field as the subquery’s first expression will penetrate to the table/cursor column — the opposite of the written limitation. There is no test for this shape, which is exactly the real-project case in #142 (v_fund_acnt_all.CLIENT_ACNT_ID).
Suggestion: Add INSERT … SELECT (SELECT r.code FROM dual) … / (SELECT v_fund_acnt_all.CLIENT_ACNT_ID …) with a seeded record_cursors context and assert a column source, not Variable. Drop or rewrite the FieldAccess limitation so it only covers true Expr::FieldAccess ((expr).field).
| // under the target column's own name. | ||
| Some(ColumnSource::Column { | ||
| table: c.source_table.clone(), | ||
| column: column.clone(), |
There was a problem hiding this comment.
[suggestion] Whole-record SELECT * catch-all attributes each INSERT column under the target column’s name (column.clone()), while field-access catch-all at 3162 uses the record field name. For INSERT INTO t_dst (amt, id) VALUES r with CURSOR cur IS SELECT * FROM t_src (columns id, amt), PL/SQL is positional (t_src.id → t_dst.amt), but this path emits t_dst.amt ← t_src.amt and t_dst.id ← t_src.id. That is silently wrong whenever the column list is reordered or renamed; there is no whole-record + SELECT * test at all (unit test only covers explicit cursor columns in matching order).
Suggestion: Either skip catch-all whole-record expansion when the target names cannot be proven to match SELECT * order (leave unmapped, matching the documented DDL-order limitation), or document that this heuristic is name-based on the INSERT list. Add a test for SELECT * + VALUES r with a matching column list, and one with a reordered list if you keep the heuristic.
| } | ||
| } | ||
| } else { | ||
| // #142: the `%ROWTYPE` anchor is a TABLE, not a registered |
There was a problem hiding this comment.
[suggestion] Several new comments narrate the change and embed design history rather than a non-obvious constraint: the table-anchor block (3168–3172) explains “unlike the old ?.field” and custom TYPE rarity; push_column_mapping (3363–3366) restates the call-site list already in the issue; RecordVariable (2931–2936) is closer to a useful limitation note but still leads with #142 play-by-play. Project convention is short WHY comments, not architecture history.
Suggestion: Keep one line where the constraint is non-obvious (e.g. “table-anchored %ROWTYPE has no cursor_sources entry; attribute to the type name” / “JOIN walk of collect_aliases_from_table_refs is not restored”). Drop issue numbers, the old-behavior contrast, and the SET/MERGE call-site inventory.
|
已按审核意见逐条修复并推送（6 个 commit，均走 TDD：先加失败测试 → 最小实现 → 全量门禁）：
关于 #2 的补充：审计发现泄漏有两处来源——除 门禁： |
| let new_scope = self.scope_sole_table_of(&select.from); | ||
| let saved_scope = std::mem::replace(&mut self.scope_sole_table, new_scope); | ||
|
|
||
| let (sources, kind, expression) = match select.targets.first() { |
There was a problem hiding this comment.
[bug] push_subquery_column_mapping always classifies select.targets.first(), while position is ignored. push_column_mapping intercepts every Expr::Subquery value (not only scalar subqueries in an INSERT select-list), and visit_update already applies one assignment value to each of SET (a, b, c) = expr by enumerating assignment.columns. For UPDATE t SET (a, b) = (SELECT x, y FROM t2) both a and b therefore become a Direct copy of t2.x. Before this intercept, collect_value_sources skipped Expr::Subquery and both columns stayed unmapped — empty is honest; b ← x is a silent wrong edge. The surrounding comment still says this form shares the whole expression rather than being split; the new choke point splits incorrectly instead.
Suggestion: Keep first() only for a true scalar subquery (targets.len() == 1). When the same subquery is applied to several columns and targets.len() > 1, classify select.targets.get(position) (the index visit_update already passes). If you do not want to split yet, skip the Subquery intercept when targets.len() != 1 so later columns are not aliased to the first. Add a unit test for UPDATE t SET (a, b) = (SELECT x, y FROM t2).
| Expr::Subquery(_) | ||
| | Expr::Exists(_) | ||
| | Expr::InSubquery { .. } | ||
| | Expr::ScalarSublink { .. } => return VisitorResult::SkipChildren, |
There was a problem hiding this comment.
[suggestion] visit_expr returns SkipChildren for InSubquery and ScalarSublink as well as Subquery/Exists. ogsql-parser’s walk_expr visits the left operand and then the nested SELECT; SkipChildren skips both. walk_expr_for_column_refs still walks InSubquery.expr in SELECT/WHERE/HAVING/ORDER BY, so those paths keep the left column — but it has no ScalarSublink arm (_ => {}), and process_expr_for_joins_and_filters does not handle InSubquery either. The generic walker was the only collector for t.x in WHERE t.x > ANY (SELECT …) and for t.id in ON t.id IN (SELECT …). Subquery/Exists SkipChildren is the right leak fix; folding in the container variants is broader than needed.
Suggestion: Skip only the nested SELECT. For InSubquery/ScalarSublink, walk the left operand (e.g. walk_expr_for_column_refs(expr), and extend that helper to ScalarSublink) then return SkipChildren. Mirror TableAccessExtractor::walk_expr_subqueries, which already splits expr vs subquery.
|
已按第二轮审核意见（Review 5136742683）修复并推送（2 个 commit，均走 TDD）：
验证：
|
1 participant
Summary
修复 #142 列级血缘穿透局限：游标
%ROWTYPE记录变量写入与 INSERT..SELECT 目标列表标量子查询无法解析真实源列。改动全部位于解析层src/parser/extractor.rs，不触及 store 结构（不 bumpSTORE_VERSION），血缘 walker 与 CLI 无需改动。背景
issue 报告的 0.9.0 行为经实测（当前分支 0.9.1 + #148）后确认：游标锚定
%ROWTYPE精确复现已由 #148 修复，但表锚定%ROWTYPE、SELECT *游标、整记录写入三种变体仍坏（?.col或无映射），标量子查询目标完全未修（"No column lineage"）。Changes
push_column_mapping分流 + 新增push_subquery_column_mapping）：INSERT .. SELECT (SELECT ...)/VALUES ((SELECT ...))取子查询 SELECT 列表首表达式，在子查询自身 FROM 作用域（alias map +scope_sole_tablesave/restore）下解析；相关子查询外层引用（s.id）不泄漏为源；首表达式为字面量时记为常量源。%ROWTYPE记录字段（column_sourceelse 回退）：rec t_src%ROWTYPE的字段归因为该表列。SELECT *游标 + 记录字段（column_sourcecatch-all 匹配）：catch-all 游标源下字段归因到游标表（列名取字段名）。visit_insertRecordVariable分支拆出）：INSERT INTO t (a,b) VALUES r按游标源列位置展开（含SELECT *catch-all 回退；表锚定需 DDL 列序，留作已知局限）。Testing
每个行为走 TDD 循环（失败测试 → 最小实现 → 通过 → 提交），共 4 单测 + 5 端到端回归测试（
tests/regress_column_lineage.rs含 #148 已修复形态的特征测试锁定）：%ROWTYPEVALUES (r.id,r.amt)?.id（0.9.0）t_dst.amt ← t_src.amtr t_src%ROWTYPE?.idt2_dst.id ← t2_src.idSELECT *游标?.amtt3_dst.amt ← t3_src.amtVALUES rt3_dst.id ← t3_src.idt_out.code ← t_ref.codet_src.idt_src.id（保持）门禁（AGENTS.md 命令）：
cargo fmt --all -- --check干净、cargo clippy --features full -- -D warnings干净、cargo test --features full -- --skip test_path_mapping_applied --skip test_serve_全绿。已知局限
INSERT INTO t VALUES r）需目标表 DDL 列序，静态解析无法完成；v_fund_acnt_all.CLIENT_ACNT_ID）解析为 Variable 源（优于 "No column lineage"，不穿透到列，属后续增强）；Expr::ScalarSublink（ANY/ALL/SOME谓词）非值源，不处理。Fix #142