Skip to content

fix(ddxdb): plan on DuckDB without the planner's empty-table statistics - #206

Merged
alxmrs merged 1 commit into
mainfrom
fix/duckdb-planner-stats
Oct 25, 2026
Merged

alxmrs merged 1 commit into
mainfrom
fix/duckdb-planner-stats

Conversation

@alxmrs

@alxmrs alxmrs commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

Why

A silent wrong gradient on DuckDB, in ddxdb 0.4.2. Take a loss that filters a table on a column:

WITH loss AS (SELECT SUM(w.val * p.w) AS l FROM w JOIN p ON w.i = p.i WHERE p.layer = 0)
SELECT i, grad(loss, val) FROM w
  • The gradient with respect to w.val comes out 0 everywhere, with no error. The right answer is p.w.
  • The gradient with respect to p.w is refused: ddx says the query does not read p.

The GPT's step filters every parameter table this way (WHERE p.layer = {layer}).

DuckDB's backend plans on a second connection whose tables are empty copies of the caller's. DuckDB's statistics propagation proves that the filter matches no rows of an empty table, and plans the scan as an empty result. So ddx differentiates a plan that never reads p.

How

The planner sets disabled_optimizers = 'statistics_propagation'. Nothing runs on the planner, so its plans gain nothing from statistics. They now read every table the query reads, under the query's filters. The caller's connection, where the steps run, is unchanged.

Tests

test_a_filter_on_a_table_s_column_keeps_the_table_s_rows takes both gradients of the loss above. Without the fix, it gets [0, 0, 0] for w.val.

Next

#204's fix (the GPT on DuckDB) needs this too. Worth a patch release.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CvszJhH8pn8H9G69wEPMU2

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue that could cause filtered queries on registered tables to incorrectly return no rows. Gradients now correctly reflect rows included in a filter and join, with excluded rows receiving zero gradients. This improves results when training with losses that use filtered, registered-table data.

DuckDB's backend plans on a second connection whose tables are empty copies
of the caller's. DuckDB's statistics propagation proves a filter on such a
table's column (`WHERE p.layer = 0`) matches no rows, and plans the scan as
an empty result. ddx then differentiates a plan that never reads the table.
A gradient through the join comes out 0, with no error, and a gradient with
respect to the table is refused as unknown.

The planner turns statistics propagation off. Its plans then read every
table the query reads, under the query's filters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CvszJhH8pn8H9G69wEPMU2
@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b5757e91-5bb3-4963-9301-f41038198d8d
Commits

Reviewing files that changed from the base of the PR and between 921aef1 and 0074bee.

Files selected for processing: 2
  • python/ddxdb/ddxdb/duckdb.py
  • python/ddxdb/tests/test_duckdb.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

The DuckDB planner now disables statistics propagation. A regression test checks gradients for a filtered join against a registered table.

Changes: DuckDB filtered gradients

Layer / File(s) Summary
Planner configuration and regression test
python/ddxdb/ddxdb/duckdb.py, python/ddxdb/tests/test_duckdb.py
DuckDBBackend disables statistics_propagation on the planner connection. The test checks gradients for joined w.val and registered p.w rows, including rows excluded by the filter or join.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Possibly related issues

Possibly related PRs

  • xqlsystems/ddx#192 — Adds the DuckDB backend that plans with an empty catalog mirror; this PR changes that planner’s optimizer configuration and tests the reported failure mode.
  • xqlsystems/ddx#201 — Releases the DuckDB backend implementation that this PR modifies.
Merge Risk: Minimal · up to 0074b

No actionable issue was identified that should delay merging after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the main fix: disabling DuckDB planner statistics from empty tables in ddxdb. It is concise and specific.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@alxmrs alxmrs left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, very good catch.

@alxmrs
alxmrs merged commit d88a501 into main Oct 25, 2026
16 checks passed
@alxmrs
alxmrs deleted the fix/duckdb-planner-stats branch October 25, 2026 23:39
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