fix: apply cheap WHERE conjuncts before inference - #4
Conversation
| ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute), | ||
| )?; | ||
| } | ||
| let post = FilterExecBuilder::new(conjunction(needs_inference), rebuilt) |
There was a problem hiding this comment.
The rebuilt post filter keeps only the projection. The rebuilt post filter drops the original FilterExec's fetch, batch_size and default selectivity.
Fix: copy those settings from filter onto the builder. For example, chain .with_fetch(filter.fetch()) and the matching batch size and selectivity setters. Add a test with LIMIT and target_partitions = 1.
Failure scenario: LimitPushdown runs before this rule, because custom physical rules come after the built-in rules. With one partition, LimitPushdown can fold GlobalLimitExec into FilterExec as fetch = 1 and then remove the limit node. This rule then rebuilds the filter without fetch. SELECT id FROM t WHERE id > 0 AND prompt_jev(body, 'q') > 0.5 LIMIT 1 then returns every matching row. With several partitions, a CoalescePartitionsExec fetch still caps the row count. In that case, losing the filter fetch only adds paid inference.
This bug exists only if LimitPushdown in DataFusion 55 folds the limit into FilterExec. The DataFusion source is not in this checkout, so the review did not confirm that behavior. Copying fetch is safe in both cases.
There was a problem hiding this comment.
Review — Blocking Issues: src/optimizer.rs:96: FilterBeforeJev rebuilds the post filter without the original filter's fetch, batch_size and default selectivity. LimitPushdown runs before this rule. If LimitPushdown has folded a LIMIT into the FilterExec and removed the limit node, the rebuilt plan returns too many rows. With several partitions, the missing fetch sends extra rows to paid inference.
Action Required: Copy fetch, batch size and default selectivity from the original FilterExec onto the FilterExecBuilder for the post filter. Add a test for WHERE cheap AND prompt_jev(...) > x LIMIT 1 with target_partitions = 1. The test must check the row count and the number of rows sent for inference.
`WHERE region = 'eu' AND prompt_jev(...) > 0.8` was planned as one filter above the async node, so every row was sent for inference before either conjunct ran. A physical rule now splits such a filter: conjuncts that need no async result move beneath the async node, looking through any repartition nodes the distribution rules placed in between. Found by a 1,000-query test run: 6 queries failed because a row the cheap conjunct excluded was still sent and exceeded the input limit.
7abcb5d to
6fccc68
Compare
There was a problem hiding this comment.
Review — Blocking Issues
src/optimizer.rs:96: the rebuilt postFilterExecstill drops the original filter'sfetch,batch_sizeand default selectivity. Thread #4089060353 raised this issue, and the force-pushed head 6fccc68 does not change the code. Custom physical rules run after the built-inLimitPushdownrule. Withtarget_partitions = 1,LimitPushdowncan foldLIMIT 1intoFilterExecasfetch = 1and remove the limit node. The rebuilt filter then returns every matching row. This failure depends onLimitPushdownbehavior in DataFusion 55, which the review could not inspect. Copying the settings is safe in both cases.
Action Required
- Copy
filter.fetch(), the batch size and the default selectivity fromfilteronto theFilterExecBuilderatsrc/optimizer.rs:96. - Add a test in
tests/sql.rswithLIMIT 1,target_partitions = 1and a mixedWHERE. The test asserts one returned row.
|
Addressed: the rebuilt post filter now carries the original filter's fetch, batch size, and default selectivity. Added a test with LIMIT 1 and target_partitions = 1 over a mixed WHERE that asserts one returned row and that rows failing the cheap predicate are not sent for inference. |
Problem
A 1,000-query test run passed 994. The 6 failures were all
WHERE <cheap predicate> AND prompt_jev(...) > x: DataFusion plans that as a singleFilterExecaboveAsyncFuncExec, so every row is sent for inference before either conjunct is applied. A row the cheap predicate excluded still reached the provider and tripped the 64 KiB input limit. Even without the error, it means paid inference on rows the query discards.Fix
FilterBeforeJev, a physical optimizer rule: when a filter over the async node has conjuncts that reference no async result column, those conjuncts move into a new filter beneath the async node. Repartition nodes between the two are looked through and rebuilt in place. The remaining conjuncts stay above.Plan before and after for
WHERE id = 1 AND prompt_jev(body, 'q') > 0.5:Tests
33 pass (2 new): only the row passing the cheap predicate is asked about; a
count(*)with a mixed filter asks about exactly the surviving rows.