Skip to content

Enable VARIANT pushdown extract and pin shredding schema - #32

Open
fuziontech wants to merge 7 commits into
upstream-main-2026-07-22from
variant-pushdown-extract-and-shredding
Open

Enable VARIANT pushdown extract and pin shredding schema#32
fuziontech wants to merge 7 commits into
upstream-main-2026-07-22from
variant-pushdown-extract-and-shredding

Conversation

@fuziontech

Copy link
Copy Markdown
Member

Summary

DuckLake set both statistics and statistics_extended on its scan function. RemoveUnusedColumns disables pushdown extract for any scan where function.statistics is non-null (remove_unused_columns.cpp:685-688), so every DuckLake scan silently lost VARIANT field pushdown — reading and decoding whole variants even when a query touched a single shredded field.

The capability was already shipping in our pinned DuckDB (c97bd8b96e has IsPushdownExtract in the Parquet variant reader). It was just switched off.

Changes

Read path (ducklake_scan.cpp)

  • Only set statistics_extended, mirroring the native table_scan function. All optimizer consumers prefer it when present (propagate_get.cpp:167-171, relation_statistics_helper.cpp:188-193, partitioned_execution.cpp:206-210).
  • Correctness fix that must land with it: DuckLakeStatisticsExtended took only GetPrimaryIndex() and dropped the child path. With pushdown enabled it would attribute whole-column min/max to a single extracted field and prune wrongly — wrong results, not just bad estimates. It now narrows via PushdownExtract, matching MultiFileScanStatsExtended.
  • Pushdown gated to plain table scans over Parquet: the inlined-data reader has never seen a pushdown-extract ColumnIndex.

Write path (ducklake_insert.cpp)

  • New parquet_shredding option pins the shredding schema instead of re-deriving it per file, which is what makes shredded_type consistent across files:
    CALL ducklake.set_option('parquet_shredding', 'v: ''STRUCT(a BIGINT, b VARCHAR)''');
  • Paren/quote-aware parsing so type strings containing commas survive; validated at set_option time, not at the next INSERT.
  • Settable at catalog/schema scope, so entries naming columns that aren't VARIANT in a given table are dropped rather than raising — an unusable setting degrades to "no explicit shredding" instead of a failed INSERT.
  • Compaction picks this up for free via the shared insert path.

Measured

1.5M rows / 206MB, incompressible payload, single file, same binary stashed and rebuilt:

Query Before After Gain
sum(v.a::BIGINT) 199.5 MB / 0.046s 6.0 MB / 0.011s 33× less IO, 4.2× faster
count(*) WHERE v.a = 5 199.5 MB / 0.081s 12.1 MB / 0.032s 16.5× less IO, 2.5× faster
count(v::VARCHAR) 199.5 MB / 1.31s 199.5 MB / 1.07s unchanged — by design

The last row is correct: reconstructing whole variants genuinely needs every byte. Note the gain is invisible on small files, where a single prefetch pulls the whole file regardless — an early 8MB benchmark showed no difference at all.

Testing

Full suite: 17237 assertions in 491 test cases, no failures (baseline 17169/489; the delta is the two new files).

variant_pushdown_extract.test targets the stats-attribution risk directly, using fields with deliberately divergent ranges (a ascending, b descending, c constant) so wrong attribution prunes and returns wrong counts rather than merely bad estimates. Also covers nested extract, multi-file, transaction-local, and inlined paths.

Not included

Cross-file pruning from ducklake_file_variant_stats — those rows are still written and never read, so WHERE props.a = 15000 across two files with disjoint ranges still reads both. Tracked as follow-up work on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR

DuckLake set both `statistics` and `statistics_extended` on its scan
function. RemoveUnusedColumns disables pushdown extract for any scan
where `function.statistics` is non-null, so every DuckLake scan silently
lost VARIANT field pushdown - reading and decoding whole variants even
when a query touched a single shredded field.

Only set `statistics_extended`, mirroring the native table_scan function.
All optimizer consumers prefer it when present (propagate_get.cpp,
relation_statistics_helper.cpp, partitioned_execution.cpp), and
DuckLakeStatisticsExtended covers everything DuckLakeStatistics did.

That alone would be a correctness bug: DuckLakeStatisticsExtended took
only GetPrimaryIndex() and dropped the child path, so with pushdown
enabled it would attribute whole-column min/max to a single extracted
field and prune wrongly. It now narrows via PushdownExtract, matching
MultiFileScanStatsExtended, and bails to nullptr on anything unexpected.

Pushdown is gated to plain table scans over Parquet data: the inlined
data reader has never seen a pushdown-extract ColumnIndex.

Measured on 1.5M rows / 206MB (incompressible payload, one file):

  sum(v.a::BIGINT)            199.5MB/0.046s -> 6.0MB/0.011s
  count(*) WHERE v.a = 5      199.5MB/0.081s -> 12.1MB/0.032s
  count(v::VARCHAR)           199.5MB/1.31s  -> 199.5MB/1.07s

The last row is unchanged by design - reconstructing whole variants
genuinely needs every byte. The gain is invisible on small files, where
a single prefetch pulls the whole file regardless.

Also adds a `parquet_shredding` option so the shredding schema can be
pinned instead of re-derived per file, which is what makes shredded_type
consistent across files:

  CALL ducklake.set_option('parquet_shredding',
                           'v: ''STRUCT(a BIGINT, b VARCHAR)''');

Parsed with a paren/quote-aware splitter so type strings containing
commas survive, and validated at set_option time rather than at the next
INSERT. The option can be set at catalog/schema scope, so entries naming
columns that are not VARIANT in a given table are dropped rather than
raising - an unusable setting degrades to "no explicit shredding"
instead of a failed INSERT. Compaction picks this up for free via the
shared insert path.

variant_pushdown_extract.test targets the stats-attribution risk
directly, using fields with divergent ranges (a ascending, b descending,
c constant) so that wrong attribution prunes and returns wrong counts
rather than merely bad estimates.

Full suite: 17237 assertions in 491 test cases, no failures.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR
Comment on lines +15 to +48
name: Check Code Format
runs-on: ubuntu-22.04
env:
CC: gcc-10
CXX: g++-10
GEN: ninja

steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
submodules: 'true'

- name: Checkout DuckDB to version
run: |
cd duckdb
git checkout $(cat ../.github/duckdb-version)

- name: Install formatting tools
run: |
sudo apt-get update -y -qq
sudo apt-get install -y -qq ninja-build python3-venv
python3 -m venv .cache/format-venv
.cache/format-venv/bin/python -m pip install cmake-format 'black==24.*' cxxheaderparser pcpp 'clang_format==11.0.1'

- name: List Installed Packages
run: .cache/format-venv/bin/python -m pip freeze

- name: Check format
run: |
.cache/format-venv/bin/clang-format --version
.cache/format-venv/bin/clang-format --dump-config
.cache/format-venv/bin/black --version
.cache/format-venv/bin/python duckdb/scripts/format.py --all --check --directories src test
Comment on lines +91 to +131
relassert:
name: Build relassert
runs-on: ${{ github.repository_owner == 'duckdb' && 'namespace-profile-linux-x64' || 'ubuntu-latest' }}
env:
GEN: ninja
CC: clang-20
CXX: clang++-20
# Enable -DDEBUG slow verifiers (expression verifier, operator
# round-trips, etc.) without changing the optimization level.
FORCE_DEBUG: 1
FORCE_ASSERT: 1
ASAN_OPTIONS: "detect_leaks=1:halt_on_error=1:abort_on_error=0"
UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1"
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
submodules: 'true'
- name: Install tools
run: python3 duckdb/scripts/ci/retry.py -- make -C duckdb toolsci
- name: Fix ASan runtime
# libclang_rt.asan_static.a is missing in ubuntu 24.04 — workaround
# mirrored from upstream Main.yml.
run: |
asan_static_dir="/usr/lib/llvm-20/lib/clang/20/lib/x86_64-pc-linux-gnu"
asan_static_target="/usr/lib/llvm-20/lib/clang/20/lib/linux/libclang_rt.asan_static-x86_64.a"
if [[ ! -e "${asan_static_dir}/libclang_rt.asan_static.a" && -e "${asan_static_target}" ]]; then
sudo mkdir -p "${asan_static_dir}"
sudo ln -sf "${asan_static_target}" "${asan_static_dir}/libclang_rt.asan_static.a"
fi
- name: Setup Ccache
uses: hendrikmuhs/ccache-action@main
with:
max-size: 5GB
evict-old-files: job
- name: Build (relassert → ASan + UBSan + LSan)
uses: duckdb/duckdb-ci/build-duckdb@main
with:
build-type: relassert
build: python3 duckdb/scripts/ci/retry.py -- make relassert
test: make unittest_relassert
@fuziontech
fuziontech changed the base branch from main to upstream-main-2026-07-22 July 28, 2026 14:21
@fuziontech

Copy link
Copy Markdown
Member Author

Note on the base branch — needs a decision

This PR targets upstream-main-2026-07-22 (upstream eb7b95df), not main, because of a DuckDB pin problem worth flagging.

Branch DuckDB pin Date Has variant reader-side pushdown?
main bcb0c9c5 2026-05-04
posthog/v1.5.3 (active line) 14eca11b = tag v1.5.3 2026-05-19
this PR's base c97bd8b96e 2026-07-22

IsPushdownExtract / GetVariantExtractPath in extension/parquet/reader/variant_column_reader.cpp are absent from both of our pins. That code landed on duckdb main in July (PR duckdb/duckdb#22478) and never made the v1.5.x release line — the same split that makes released v1.5.5 behave differently from main here.

Consequence: enabling pushdown extract on a pin that lacks the reader-side consumption would be untested, and plausibly unsafe — the optimizer would push VARIANT extracts into a reader with no handling for them. So this cannot simply be rebased onto main or posthog/v1.5.3 as-is.

Targeting main directly produced a 257-file diff, because it also dragged in the 477 upstream commits between our fork point and July upstream. Retargeting isolates it to the 8 files actually changed here.

Options:

  1. Land on a July-upstream-based line (what this PR does) and advance the fork's pin separately.
  2. Advance posthog/v1.5.3's DuckDB pin to a main-based commit first, then rebase — larger change, since v1.5.x → main is not a small jump.
  3. Hold until the fork syncs with upstream main generally.

Numbers in the description were measured on the c97bd8b96e pin; they will not reproduce on either current fork pin.

ducklake_file_variant_stats records per-path min/max for every data file,
but nothing ever read it: a filter on a field inside a VARIANT opened
every file in the table. Two files with disjoint ranges for props.a both
got scanned for `WHERE props.a = 15000`.

The sub-path was being dropped on the way in. ComplexFilterPushdown
already receives info.column_indexes, where a pushdown-extract
ColumnIndex carries the field chain, but AddFilterToPushdownInfo only
took the plain column_id. It now takes the ColumnIndex and reconstructs
the path with QuoteVariantFieldName - the same encoding the stats writer
uses, so the lookup key matches by construction.

DynamicFilterPushdown built a fresh FilterPushdownInfo rather than
copying the existing one, which silently discarded any path-qualified
filter static pushdown had already resolved. It now seeds from
filter_info the way ComplexFilterPushdown does; that interface only gets
column_ids and cannot rebuild a path itself.

Two correctness traps, both of which produce wrong answers rather than
loud failures, and both caught by tests here:

1. The bounds guard has to cast to the type the comparison casts to (the
   constant's type), not the column's declared type. Using the declared
   VARIANT type degraded the guard to `min_value IS NULL`, which never
   fires, and `WHERE v.a::BIGINT = 5` pruned every file and returned 0
   where the answer was 1.

2. min_value/max_value are written in the ordering of whatever type the
   path was shredded as, and that differs per file. A path shredded as
   varchar has string-ordered bounds - min='10', max='9' - so casting
   those to BIGINT and comparing numerically prunes files that really do
   contain matches. Pruning is now restricted to shredded types sharing
   an ordering domain with the constant: any integer width for an
   integer constant, exact match otherwise, and no pruning at all for
   decimals, blobs and nested types.

Only a single comparison against a constant is recognised. Conjunctions
and IS NULL are skipped, the latter deliberately: for a VARIANT a
missing key, an explicit VARIANT_NULL and a SQL NULL are three different
things that null_count cannot distinguish. Filters that reach the scan
as an opaque variant_comparator sort-key blob are also left unpruned.

Everything unrecognised fails open - the file is kept and the scan
filters it as before. Files with no stats row for a path (never shredded
there) fall outside the CTE and are likewise kept.

Bucket-partition pruning now skips path-qualified filters; it would
otherwise hash the wrong values.

Measured on 20 files / 66MB, same binary, identical results:

  v.a::BIGINT = 500005   1/20 files    201 kB   0.013s
  v.a = 500005 (opaque)  20/20 files   4.0 MB   0.027s

Full suite: 17277 assertions in 492 test cases, no failures.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR
@fuziontech

Copy link
Copy Markdown
Member Author

P1 added: file pruning from ducklake_file_variant_stats (7af9617)

That table has always been written per data file and per path, and nothing ever read it — a filter on a VARIANT field opened every file in the table.

Where the path was lost. ComplexFilterPushdown already receives info.column_indexes, and a pushdown-extract ColumnIndex carries the field chain; AddFilterToPushdownInfo just took the plain column_id and dropped it. It now takes the ColumnIndex and rebuilds the path via QuoteVariantFieldName — the same encoding the stats writer uses, so the lookup key matches by construction. Separately, DynamicFilterPushdown built a fresh FilterPushdownInfo instead of copying the existing one, discarding any path-qualified filter static pushdown had already resolved.

No upstream change was needed after all — column_indexes is already on MultiFilePushdownInfo.

Two traps, both caught by tests

Both produce wrong answers rather than loud failures, which is why the tests are written to fail on the wrong count rather than on an error.

  1. Guard cast type. The bounds guard must cast to the type the comparison casts to (the constant's), not the column's declared type. Using the declared VARIANT type degraded it to min_value IS NULL, which never fires — WHERE v.a::BIGINT = 5 pruned every file and returned 0 where the answer was 1.

  2. Ordering domain. min_value/max_value are written in the ordering of whatever type the path was shredded as, which differs per file. A varchar-shredded path has string-ordered bounds — min='10', max='9' — so casting those to BIGINT and comparing numerically prunes files that genuinely contain matches. Pruning is now restricted to shredded types sharing an ordering domain with the constant: any integer width for an integer constant, exact match otherwise, and nothing at all for decimals, blobs, nested types.

Deliberately not pruned

Only a single comparison against a constant is recognised. Conjunctions are skipped, and IS NULL is skipped on purpose: for a VARIANT, a missing key, an explicit VARIANT_NULL, and a SQL NULL are three different things that null_count cannot distinguish. Filters that arrive as an opaque variant_comparator sort-key blob — which is what the bare v.a = 5 form lowers to — are also left alone; decoding that encoding to compare against VARCHAR-stored bounds is a separate piece of work.

So today pruning applies to the cast form (v.a::BIGINT = 5), not the bare form. Everything unrecognised fails open.

Measured — 20 files / 66 MB, same binary, identical results

Filter Files read Data read Time
v.a::BIGINT = 500005 (pruned) 1 / 20 201 kB 0.013s
v.a = 500005 (opaque blob form) 20 / 20 4.0 MB 0.027s

Both return 1.

Full suite now 17277 assertions in 492 test cases, no failures.

Sources DuckDB from PostHog/duckdb rather than duckdb/duckdb, tracking
posthog/main-snapshot-2026-07-22. The pinned commit is unchanged
(c97bd8b96e) - only the repository it is fetched from moves - so the
build and all measurements in this branch are unaffected.

The VARIANT extract pushdown this branch depends on exists only on duckdb
main. It is not on the 1.5 line and upstream is not backporting it:
duckdb's v1.5-variegata is 20 commits past v1.5.5 and still ships the
pre-rewrite reader. Between v1.5.5 and main the reader was rewritten
rather than extended, so test-applying the pushdown PR onto v1.5.5
conflicts in 21 files.

Pinning a named snapshot branch in our own fork gives us a stable, and
patchable, base for that dependency instead of tracking a moving
upstream main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR
@fuziontech

Copy link
Copy Markdown
Member Author

Base resolved: PostHog/duckdb fork (0dcccaa)

PostHog/duckdb is forked, with a pinned snapshot branch posthog/main-snapshot-2026-07-22 at c97bd8b96e — verified to carry IsPushdownExtract in the Parquet variant reader.

This branch's .gitmodules now sources DuckDB from PostHog/duckdb instead of duckdb/duckdb. The pinned commit is unchanged, so nothing in the build or the measurements above moves; only the repository it is fetched from. Verified the submodule resolves from the fork.

Pinning a named snapshot in our own fork gives a stable and patchable base, rather than tracking a moving upstream main.

Why not v1.5.5

Recorded here since it was the obvious alternative and it does not work. See also #33, which bumps the 1.5 line to v1.5.5 on its own merits.

  • v1.5.5 ships the pre-rewrite reader (variant_shredded_conversion.cpp), no pushdown.
  • duckdb's v1.5-variegata is 20 commits past v1.5.5 and still ships that same reader — upstream is not backporting to the 1.5 line.
  • Between v1.5.5 and main the reader was rewritten, not extended: variant_shredded_conversion.cpp (596 ln) deleted, replaced by ParquetVariantIterator (715) + parquet_variant_shredding.cpp (396); variant_binary_decoder.cpp 465 → 124 ln.
  • Test-applying the pushdown PR ([MultiFileReader][Parquet] Add variant_extract pushdown duckdb/duckdb#22478) onto v1.5.5 conflicts in 21 files.

Trade-off being accepted

The base is a pre-release main snapshot, and this branch sits on ducklake main rather than the 1.5 line — so the 35 posthog/v1.5.3-only commits would need forward-porting before this can serve as that line's replacement. That is the known cost of this route; the alternative was owning a divergent variant subsystem indefinitely.

fuziontech and others added 4 commits July 29, 2026 09:29
The base branch gained SHA-pinned GitHub Actions (PostHog's org ruleset
rejects unpinned refs). Merging it here so this branch's merge ref
actually uses those workflows - updating the base alone does not
re-trigger PR checks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR

* 'upstream-main-2026-07-22' of https://github.com/PostHog/ducklake:
  Pin GitHub Actions to SHAs so CI can run on this fork
Check Code Format flagged the AddFilterToPushdownInfo declaration: after
adding the ColumnIndex parameter the wrap point no longer matched
clang-format 11.0.1. Purely a line rewrap, no semantic change.

This job never ran on this branch before - every job was failing at setup
on the unpinned-actions ruleset, so the formatting problem was masked.

Verified all ten changed source files are clean under the repo's pinned
clang-format 11.0.1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR
Enabling pushdown extract made prefetch accounting count a VARIANT
column's chunks once per extracted field, tripping the "incorrectly set
page offsets" guard. Local files silently disable prefetching instead of
throwing, so this only showed up over S3 - every local test and non-MinIO
CI job passed while Tests using MinIo failed on
variant_shredded_stats.test:229 (three extracts) and
parquet_shredding.test:51 (two).

Points the submodule at PostHog/duckdb
posthog/fix-variant-pushdown-prefetch-accounting (5439ec70), which counts
each chunk once. Those two MinIO tests are the regression coverage.

Full suite still passes locally: 17277 assertions in 492 test cases.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR
The submodule pin alone had no effect on CI. Every test workflow does

    submodules: 'true'
    git checkout $(cat ../.github/duckdb-version)

inside the submodule, so it checks out whatever that file names and
discards the pinned commit. MinIO kept failing on the page-offset guard
because it was still building c97bd8b96e, without the prefetch-accounting
fix. This is the same trap as the v1.5.5 bump, where the version file -
not the submodule - is what CI actually honours.

duckdb-version now names the fix commit (5439ec70). It already held a raw
SHA rather than a tag, so no format change is needed, and `git checkout`
resolves it because .gitmodules fetches the submodule from our fork.

The distribution job additionally needs override_duckdb_repository: that
commit exists only in PostHog/duckdb, so cloning from duckdb/duckdb would
fail to resolve it. extension-ci-tools exposes the input for exactly this.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Mntu6YhMuqNBFD9snXqvBR
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.

2 participants