Skip to content

Represent scan snapshots explicitly instead of parsing table names - #19

Merged
ila merged 1 commit into
mainfrom
structured-snapshot-pins
Sep 21, 2026
Merged

ila merged 1 commit into
mainfrom
structured-snapshot-pins

Conversation

@ila

@ila ila commented Sep 21, 2026

Copy link
Copy Markdown
Member

Thanks for reviewing.

Why

LPTS currently appends AT (...) to a table-name string and later reparses that string to render a snapshot. This confuses identifiers with SQL: an ordinary table named events AT (VERSION => 2) is incorrectly treated as a time-travel scan.

OpenIVM also needs to restore foreign snapshot metadata after binding against local schema-only tables. Doing that in a separate AST walk duplicates traversal and relies on the same string convention.

Change

  • Carry a parsed AtClause separately on scan AST/CTE nodes; render it in DuckDB or Spark syntax at serialization time.
  • Remove the table-name suffix splitting/quoting helpers.
  • Add an optional snapshot resolver to LogicalPlanToAst, invoked during its existing GET visit with the original catalog entry, before output-name remapping affects identity. Native DuckLake snapshot metadata takes precedence.
  • Extend the existing time-travel and Spark dialect tests for escaped identifiers, alias column lists, merged/unmerged rendering, unqualified output, and identifiers that resemble snapshot syntax.

Production code is 49 lines smaller; tests add 78 lines. No new settings, additional plan walk, or handwritten SQL parser.

Validation

  • LPTS time-travel tests: 83 assertions passed.
  • Spark dialect tests: 69 assertions passed, including actual lpts_check bag-equality validation of the quoted-name regression.
  • Full LPTS SQL suite: 2,352 assertions across 46 cases passed; the initially skipped TPC-H dependency was then installed and that case separately passed all 24 assertions (all 47 cases exercised).
  • make test DuckDB corpus gate: all 3,346 files completed, no new WRONG or FAIL against baseline.
  • Consumer integration in OpenIVM PR Openivm postgres implementation #10: time-travel suite passed 214 assertions, including batched mixed DML followed by refresh and bidirectional EXCEPT ALL. Broader OpenIVM suite passed 10,577 assertions across 88 cases; one ICU-dependent case was skipped by that runner.
  • SQLStorm SF0.001 is still running. A baseline comparison is being prepared to distinguish existing errors/timeouts from regressions. Keeping this draft until that check and final review finish.

The OpenIVM consumer update is being prepared separately on ila/openivm#10; it removes the duplicate input scanner and uses this resolver during the existing LPTS traversal. Some text-only OpenIVM refresh paths still retain their existing restoration scanner; this LPTS change does not claim to eliminate those paths.

@ila
ila marked this pull request as ready for review September 21, 2026 14:40
@ila
ila merged commit 29c606e into main Sep 21, 2026
10 checks passed
@ila
ila deleted the structured-snapshot-pins branch September 21, 2026 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant