Repository navigation
Upgrade LPTS to DuckDB 2.0 and stabilize recursive checking - #20
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
LPTS converts DuckDB logical plans back to SQL. DuckDB 2.0 changes the bound-expression and scan-filter APIs and the shapes of CTE, recursive, pruned set-operation, and scan plans. Updating only the DuckDB pin would leave LPTS unable to build or would produce SQL that reparses with different bindings or types. The port also exposed two practical problems: the round-trip checker could hang on recursive queries that DuckDB itself completes, and repeated inlined CTEs could generate hundreds of kilobytes of SQL.
DuckDB 2.0 port: what broke and how it works now
ExpressionFilterform, distinguishes fixed-sizeARRAYfromLIST, and delegates optimizer-inserted comparison casts/collations to DuckDB's bound-expression SQL exporter. That last step makes the emitted SQL rebind to the same types rather than merely look similar.UNIONinputs, and hidden output columns. Without these repairs, SQL could parse successfully but read a different column or return a different number of duplicate rows.The largest source changes are in
lpts_ast_builder.cppandlpts_expression_renderer.cppbecause the 2.0 plan and expression APIs changed at many operator boundaries. The CTE serializer and checker changes below account for the other substantial additions. Tests cover round-trip results, not just successful parsing.Recursive checking and SQL size
MATERIALIZEDCTEs within one SQL statement and snapshot, then compare both bags withEXCEPT ALLin both directions. This preserves the query result and avoids the fused recursive pipeline. Nonrecursive checks retain their existing fast path.make test_releasegate; duplicated query text was removed fromcte.test.Validation at
a376261eGEN=ninja make format-fixandgit diff --checkpassed.GEN=ninja make test: 47 LPTS SQL test files, the ten pinned SQLStorm cases, and the 4,755-file DuckDB corpus gate passed with no new WRONG or FAIL.CI annotation cleanup at
fe2096dDataChunk::SetValue/SetCardinality,Vector::Flatten(count), andKeywordHelper::WriteOptionallyQuotedcalls with their supported equivalents. SQL identifier quoting is unchanged.make testrun could not complete its two DuckLake tests because theirINSTALL ducklakestep requires network access; PR CI will rerun them.ENABLE_EXTENSION_AUTOLOADINGandENABLE_EXTENSION_AUTOINSTALLcome from the pinned upstreamextension-ci-toolsMakefile, which still passes flags that DuckDB 2.0 ignores. LPTS itself does not set those flags.OpenIVM's DuckDB 2.0 port, ila/openivm#146, pins this commit and should merge after this PR.
Diff size against main
git diff --numstat 29c606ee...a376261e; additions and deletions are separate.src/andbenchmark/count as source;test/as tests;docs/and README as docs.