fix(rfc): apply every predicate, and push far more of them to SAP - #128
Merged
Conversation
…TABLE
Only `=` and `IN` with at most five values reached the SAP server. Everything
else was evaluated by DuckDB *after* the whole column had crossed the wire, so a
date-window predicate on a large fact table transferred every row and then threw
almost all of them away. On big extracts that transfer dominates every other cost
in the scan.
Most of what was needed already existed and was switched off:
- TransformComparision already mapped all six comparison operators; a guard in
TransformFilter discarded everything but `=`.
- CONJUNCTION_AND returned nothing, which is how DuckDB expresses BETWEEN on a
single column -- so a range window pushed down as nothing at all.
- CreateExpression, written to join a column's filters with an operator, was
never called.
Conjunctions are now translated all-or-nothing. Dropping one arm of an AND merely
widens the predicate and DuckDB re-filters, but dropping one arm of an OR narrows
it and would silently lose rows; rather than carry two rules, neither is emitted
partially. The IN cap becomes a length budget instead of an arbitrary count, and
an over-long list is dropped rather than truncated, since a truncated IN list is a
different, narrower predicate.
Two correctness fixes found along the way:
- CONSTANT_COMPARISON built its literal with Format("'%s'"), skipping
TransformLiteral and therefore not doubling embedded apostrophes. A value like
O'Brien closed the literal early and changed what the predicate meant.
- TransformComparision emitted `!=`, which ABAP's dynamic WHERE does not accept.
The OPTIONS chunker is now quote-aware. Walking back to the nearest whitespace
lands inside a literal whenever one contains spaces, leaving an unbalanced
apostrophe on both lines; it now breaks only at whitespace outside a literal.
Literals too long for one 70-char line are declined at generation time, where
declining is still free.
Adds erpl_rfc_pushdown_filters (default on) as an escape hatch for SAP releases
that reject the generated syntax, and as the oracle for the live equality tests:
pushdown is a pure optimisation, so the same query with it on and off must return
the same rows.
Covered by rfc/test/cpp/test_read_table_filters.cpp, which needs no SAP system.
sap_read_table sets filter_pushdown = true, which DuckDB documents as "if NOT supported a filter will be added that applies the table filter directly" -- so setting it true makes the scan solely responsible for every predicate handed to it. EXPLAIN confirms it: there is no FILTER operator above SAP_READ_TABLE. erpl translated what RFC_READ_TABLE could express and silently ignored the rest, so any predicate that did not translate returned unfiltered rows. Against the trial, `WHERE SEATS_MAX > 350` on /DMO/FLIGHT returned all 40 rows instead of 14, and a BETWEEN over FLIGHT_DATE returned 40 instead of 20. Every range, conjunction and over-wide IN was affected, which is to say most predicates that matter on a large table. Widening the translation (previous commit) shrinks the exposed set but cannot close it: literal types with no established ABAP spelling, over-long IN lists and IS NULL still cannot go to the server. Those are now evaluated here instead, so the contract holds whether or not a predicate reaches SAP. An unrecognised filter kind keeps the row rather than dropping it. That over-produces, which is what happened before this existed; dropping would be a new way to be wrong. This also makes erpl_rfc_pushdown_filters honest. It now changes only where the predicate is evaluated, never which rows come back -- without this it silently disabled filtering altogether, which is exactly how the bug was found. Verified against the trial: ground truth, pushdown off and pushdown on now all return 14 and 20 respectively.
Pushdown is only safe if it changes where a predicate is evaluated, never which rows come back. erpl_rfc_pushdown_filters is the oracle: every predicate runs twice and the two results are compared with symmetric EXCEPT ALL, which preserves multiplicity and so catches dropped rows, duplicated rows and corrupted values in one assertion. COUNT(*) would pass a drop-one/duplicate-one bug. Row order is deliberately not asserted -- RFC_READ_TABLE guarantees none. Each case also asserts the filter actually selects a strict subset, so that 'both sides agree' cannot be satisfied by returning everything, which is precisely the failure this was written to catch.
RFC_READ_TABLE's OPTIONS parser rejects the whole clause if it names the client field -- 'The parser produced the error: The client field "M..."'. The client is implicit in the RFC call, so it must not appear in a generated WHERE. A join on MANDT produces exactly such a predicate, which is what the old 'for joins, don't push down complex AND expressions' early return was really avoiding. Widening conjunction pushdown removed that accidental shield and broke the SFLIGHT/SPFLI join in sap_read_table.test. Client columns are identified from the DDIC DATATYPE at bind time, which is the only place the distinction survives: GetRfcTypeForFieldMeta maps CLNT onto RFCTYPE_CHAR, so by then it is indistinguishable from an ordinary CHAR field. The filter is routed to the residual path rather than dropped, so the rows are still filtered -- just on this side.
Records what now reaches SAP and what deliberately does not, that anything not pushed is still applied, and how literals are rendered per DDIC type. Also gitignores the local agent scratch directories, which is how they came to be staged here in the first place.
jrosskopf
force-pushed
the
claude/rfc-filter-pushdown
branch
from
August 30, 2026 16:40
4defa66 to
aed11ca
Compare
A table function signals end-of-scan by returning a chunk with no rows. Residual filtering can legitimately reject every row of a batch, and returning that empty chunk terminated the scan and silently discarded everything still unread. Only visible at scale: /DMO/FLIGHT is 40 rows and never fills a vector, so the whole suite passed. On DD03L the scan stopped at the first fully-rejected batch and returned 25,578 rows instead of 114,566. The scan now keeps pulling until a batch has a surviving row or the table is genuinely exhausted, resetting the chunk in between -- ApplyResidualFilters slices it, leaving dictionary vectors that Step would otherwise write through as if they were flat. Verified on DD02L (164,664 rows, ~80 batches): ground truth, residual and pushed all return exactly 40,786 rows with zero multiset difference in both directions.
Every other case in this file runs against /DMO/FLIGHT, which has 40 rows and never fills a single vector -- so none of them touch the multi-batch path, and all of them stayed green while the scan was truncating results. DD02L is ~165k rows across ~80 batches and TABCLASS clusters by table name, so whole batches contain no matching row. That is the shape that broke: residual filtering emptied a batch, the scan returned a zero-row chunk, and DuckDB read it as end-of-scan. The row count is deliberately not written down -- it drifts with the trial's repository contents. What is asserted is that ground truth, residual and pushed agree as multisets, that the scan really does span many batches, and that the filter rejects a large majority so empty batches actually occur.
SAP TIMS is second-precision; DuckDB TIME is not. Truncating the fraction changes the predicate rather than approximating it: `t < TIME '09:05:03.500'` must keep a row stored as 09:05:03, but the truncated clause `t < '090503'` rejects it. `>=` fails the mirror-image way. Rounding correctly would have to depend on the comparison operator, which TransformLiteral cannot see, so it declines and the residual path applies the predicate exactly. Found by an external review of the diff. Also records why OPTIONAL_FILTER is enforced in residual evaluation rather than skipped. DuckDB's own OptionalFilter::FilterSelection is a no-op, but that is a performance choice, not permission to violate the predicate: CheckStatistics delegates to the child for zone-map pruning and table_scan.cpp feeds it into index-scan comparison extraction, both of which eliminate rows. DuckDB can skip the exact check because something downstream re-applies it; here the filter was removed from the plan, so nothing would.
Collaborator
Author
Verification pass: external review + testing at scaleThe original testing was thinner than it looked — every live test ran against Second correctness bug, found only at scaleA table function signals end-of-scan by returning a chunk with no rows. Residual On DD03L this returned 25,578 rows instead of 114,566. Every existing test Fixed in External review findings
Review also confirmed: the Performance, now measured rather than asserted165k-row table, debug/ASAN build (the ~1.8s floor is fixed startup cost):
Coverage now
|
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
1 participant
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.
Phase 2 of the large-dataset extraction work. Started as a performance change and
uncovered a correctness bug on the way.
The correctness bug (found by the on/off oracle)
sap_read_tablesetsfilter_pushdown = true. DuckDB documents that as "if NOTsupported a filter will be added that applies the table filter directly" — so
setting it true makes the scan solely responsible for every predicate it
receives.
EXPLAINconfirms there is noFILTERoperator aboveSAP_READ_TABLE.erpl translated what
RFC_READ_TABLEcould express and silently ignored the rest.Anything that did not translate was simply never applied:
Every range, conjunction and over-wide
INwas affected — which is most of thepredicates that matter on a large table. Predicates that cannot reach the server
are now evaluated by erpl instead, so the result is identical either way.
The performance change
Only
=andINwith at most five values reached SAP. Everything else crossedthe wire in full. Most of what was needed already existed and was switched off:
TransformComparisionalready mapped all six operators; a guard discardedeverything but
=.CONJUNCTION_ANDreturned nothing — which is how DuckDB expressesBETWEEN, soa date window pushed down as nothing at all.
CreateExpression, written to join a column's filters, was never called.Conjunctions translate all-or-nothing: dropping one arm of an
ANDmerely widensthe predicate, but dropping one arm of an
ORnarrows it and would lose rows.The
INcap is now a length budget, and an over-long list is dropped rather thantruncated, since a truncated
INis a different, narrower predicate.Two SAP-side rules learned the hard way
DATEpushed as2020-01-01gets"is not a valid value for D(8,0)".DATSwantsYYYYMMDD,TIMSwantsHHMMSS. Types with no unambiguous ABAP spelling (TIMESTAMP,BLOB,FLOAT/DOUBLE) are declined rather than guessed at.OPTIONS.RFC_READ_TABLErejects thewhole clause. A join on
MANDTproduces exactly that, which is what the old"for joins, don't push down complex AND expressions" early return was really
avoiding — removing it broke the
SFLIGHT/SPFLIjoin. Client columns are nowidentified from the DDIC
DATATYPEat bind time, the only place the distinctionsurvives.
Also fixed: comparison literals were not escaped, so a value containing an
apostrophe closed the ABAP literal early; inequality was emitted as
!=, whichABAP does not accept; and the 70-char
OPTIONSsplitter broke inside quotedliterals containing spaces.
Testing
rfc/test/cpp/test_read_table_filters.cpp— 18 cases, no SAP system needed.Verified genuinely red against the pre-fix behaviour (7 of 13 cases failed
before the type tests were added).
rfc/test/sql/sap_read_table_filter_pushdown.test— runs each predicate witherpl_rfc_pushdown_filterson and off and compares with symmetricEXCEPT ALL,which preserves multiplicity and so catches drops, duplicates and corrupted
values in one assertion.
COUNT(*)would pass a drop-one/duplicate-one bug.Order is deliberately not asserted —
RFC_READ_TABLEguarantees none.erpl_rfc_pushdown_filters(default on) disables the translation. It changes onlywhere a predicate is evaluated, never which rows come back.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.