fix(reader): keep column-free conjuncts in the row filter - #20
Conversation
There was a problem hiding this comment.
Traced the change through the reader: LiquidRowFilter never reaches parquet-rs (opener.rs → liquid_stream.rs → liquid_cache_reader.rs), so an empty ProjectionMask on a candidate is safe — plan_row_group unions it into cache_projection as a no-op, evaluate_selection_with_predicate short-circuits to the array-less batch, and read_from_cache already handled an empty projection. The try_new_with_options switch in liquid_cache_reader.rs is equivalent for the non-empty case and slightly stricter (row count is now validated rather than inferred from the first column).
Tests cover both the cold and warm passes, and the ternary-partition assertion is the right shape for this bug. Two non-blocking notes inline.
Note I did not run anything — the Rust CI checks were still queued when this review started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📊 Benchmark ComparisonCurrent:
Compared Liquid vs DataFusionDefault on the same runner |
|
Adversarial review pass. Findings I verified independently and acted on:
Also found and filed separately as #21: nested-column conjuncts hit the same silent-drop path and are still wrong ( Considered and rejected: constant-folding the conjunct at file open. It would take the never-true query to ~50 µs by skipping the file, but it only helps a degenerate query shape and adds a plan-time evaluation path, so the ordering change carries the weight at a fraction of the complexity. |
There was a problem hiding this comment.
Prior cycle's blocking-adjacent nit is addressed: cache/mod.rs no longer .unwrap()s the batch construction or the predicate evaluation — an untyped NULL conjunct now surfaces as Some(Err(..)) instead of a panic. The Literal-first priority change is a sound heuristic given build_predicate_filter breaks once the selection empties. One super nit inline on test coverage; not blocking.
There was a problem hiding this comment.
Prior cycle's nits are all addressed in code: the batch-construction and evaluation .unwrap()s in src/datafusion/src/cache/mod.rs now propagate as Some(Err(_)), the build_row_filter doc comment no longer claims dropping conjuncts is harmless, and the cold/warm loop is back in ternary_partitions_cover_every_row_once with per-pass labels. No blocking issues.
Note that CI was still queued/in progress when this review ran, so I have no test results to point to.
Ports the code half of #20, which the sync dropped. `749b6ef` brought #20 and #40 across as tests only, on the finding that upstream had superseded both. That was right for #40 and wrong for #20: upstream refuses nested columns and columns outside the file schema through `try_pushdown_filters`, which is what those tests cover, but it still drops a conjunct that references no column at all. Nothing tested that case, so the gap looked closed. A literal `Boolean(NULL)` conjunct is exactly what expression simplification leaves behind: `NOT (s = s)` becomes `s IS NULL AND NULL`. `pushdown_columns` returns an empty column set for it, the `is_empty()` bail dropped it, and by then DataFusion has removed the `FilterExec` on the strength of the predicate being fully pushed down — so the scan is the only place it is applied and it applies a strictly weaker filter. `SELECT s FROM t WHERE NOT (s = s)` returned every row with a NULL `s` instead of none. Keep the conjunct. A column-free candidate now builds with an empty projection mask, and the cached path evaluates it against a batch that carries only the row count the selection implies — `RecordBatch` needs that explicitly, since no array is there to imply it. Found by runtimedb's query fuzzer as a ternary-partition violation: the union of `P`, `NOT P` and `P IS NULL` returned 14664 rows where the unfiltered scan returned 12000. The new tests cover both that identity and the direct `NOT (s = s)` case, cold and warm; both fail without this change. The defect is upstream's and predates the sync — our fork carried this fix, upstream still does not.
Fixes #19. The row-filter builder dropped any pushed-down conjunct whose column set was empty, so DataFusion's
s IS NULL AND NULLrewrite ofNOT (s = s)lost itsNULLconjunct and the scan — the only place the predicate is applied — matched rows where the predicate is UNKNOWN.Not DuckLake-specific and not platform-specific: it reproduces on a cold cache over a plain
register_parquetfile, which rules out the buffered-mount hypothesis in the issue.🤖 Generated with Claude Code