feat(rfc): row-range partitioning and a fetch-size knob for sap_read_table - #133
Merged
Conversation
The piece that decides which rows each worker of a partitioned sap_read_table scan
reads -- and therefore the piece that loses or duplicates them if it is wrong.
Deliberately free of any RFC dependency. The equivalent logic in erpl_odp can only be
exercised through a live connection, which is why an observed threads>1 under-count
there went unexplained; this one is covered by an 8-thread test asserting that the
claimed windows tile the row space with no gap and no overlap.
Two SAP-side rules shape it, both verified against the trial rather than assumed:
* RFC_READ_TABLE rejects ROWSKIPS % ROWCOUNT != 0 outright -- the server answers
"RFC_READ_TABLE (ROWSKIPS MOD ROWCOUNT <> 0)". Every offset handed out is
therefore a multiple of the batch size, and the window size is rounded up to a
whole number of batches so that stays true forever.
* A read at or past the end returns zero rows rather than erroring, so a short read
is how a worker discovers the table is exhausted.
MAX_ROWS is applied at claim time against raw offsets, not per worker: a per-worker
limit would return workers x MAX_ROWS rows. The final window's LOGICAL count shrinks
while the caller still requests whole batches and clips locally, because a
non-aligned ROWCOUNT is rejected server-side.
ReportExhausted prevents NEW claims only. A worker already holding a window must
always be allowed to finish it -- that is precisely the invariant erpl_odp broke, and
there is a test named for it.
Also verified live before writing any of this: two disjoint ROWSKIPS windows tile a
table exactly (1000 + 1000 rows against a 2000-row read: zero overlap, zero missing,
zero extra).
GetProjectedColumnName() scanned column_state_machines to map a projected column index back to an RFC column index, and AddOptionsFromFilters() depends on it -- so filter pushdown was coupled to the state machines living on the bind data. A partitioned scan owns them per worker, so ActivateColumns() now also records the mapping as plain data and GetProjectedColumnName() reads that instead. Built once, read-only afterwards. No behaviour change: filter pushdown suite green, 47 C++ cases green.
TrySelectFallbackReadTableFunction reassigns read_table_function -- a std::string -- from execute time, when a string column read fails with TABLE_WITHOUT_DATA. It is the one place the bind data is written after the scan has started. Column-parallel reads already make it reachable from two tasks at once; a partitioned scan would multiply that by the worker count. A caller arriving second now sees the switch already made and reports the outcome rather than probing and writing again. Found by an external review of the partitioning design, which correctly contradicted my claim that the bind data is read-only during a scan. The other site it flagged, read_table_supports_et_data_switch, is already resolved eagerly at bind whenever any column needs string support -- which is exactly when the lazy path could be reached -- so it is left alone.
sap_read_table parallelises per COLUMN: one concurrent RFC_READ_TABLE call per
projected column, all reading the same row window in lock-step. That suits a wide
extract and does nothing at all for a narrow one -- a single-column scan issues one
call and `threads` has nothing to spread.
`partitions` splits the row space instead. Workers claim disjoint windows from a
shared scheduler and read them with ROWSKIPS/ROWCOUNT, so a narrow extract finally has
something to parallelise.
Opt-in, and not because the implementation is doubted: a partitioned scan returns rows
in worker-completion order, whereas RFC_READ_TABLE is called with GET_SORTED='X' and an
unpartitioned scan returns them sorted. That is observable, so it is a choice.
Three SAP-side facts were established against the trial before any of this was written,
rather than assumed:
* Disjoint ROWSKIPS windows tile a table exactly -- 1000 + 1000 rows against a
2000-row read gave zero overlap, zero missing, zero extra.
* ROWSKIPS % ROWCOUNT != 0 is rejected outright by the server, so misalignment fails
loudly rather than returning wrong rows.
* A read at or past the end returns zero rows rather than erroring, which is how a
worker learns the table is exhausted.
Columns are read one after another inside a worker rather than fanned out. A
partitioned scan already has one call in flight per worker; fanning out per column
would make concurrent calls workers x columns with nothing bounding the product. Row
parallelism and column parallelism are alternatives, not a product.
One real bug surfaced during testing, and only with partitioning on: MAX_ROWS hung.
The unpartitioned path trims ROWCOUNT to land exactly on the limit, but a partitioned
window cannot -- ROWCOUNT has to stay batch-aligned -- so it over-fetches the final
batch. LoadNextBatchToDuckDBColumn then clips at the limit and returns 0 for every
later call, leaving pending_records permanently above zero and the state machine
spinning in LOAD_TO_DUCKDB. Reaching the limit now ends the read.
Covered by test/sql/sap_read_table_partitions.test: the same table read serially and
at 3 and 8 partitions, compared by symmetric EXCEPT ALL; a window size small enough
to force repeated hand-offs; more workers than windows; MAX_ROWS as a scan-wide limit;
filters combined with partitioning; and partitions=0/1 behaving as unpartitioned.
Five repeated runs of the 3-way comparison were identical.
Two fixes, one found by measuring and one by an external review of the diff. **It was 2x SLOWER than not partitioning.** Measured on a release build against a 164,664-row single-column extract: 2.23s unpartitioned, 4.1s at every partition count from 2 to 16. The cause was mine: a partitioned window cannot let the batch size double -- that would break ROWSKIPS alignment -- so I pinned it at STANDARD_VECTOR_SIZE, which made a window cost 64 RFC round-trips where the unpartitioned path's geometric warm-up reaches 32768 and needs a handful. The batch size is now the same one the unpartitioned path warms up to, resolved once before any worker starts. Windows default to one batch each, which is the finest granularity that still issues full-size calls, so workers share the table evenly instead of one taking a huge window while the rest idle. partitions 1 2 4 8 16 wall 2.23s 1.45s 1.11s 0.83s 0.92s 2.7x at eight workers, and the knee is visible. **ROWSKIPS could wrap.** It is an ABAP INT4 and was cast to int32_t unchecked. An unpartitioned scan reaches a large offset only by reading its way there, but a partitioned worker can START at one, so a big table or a large window setting could wrap the cast and silently re-read an earlier range -- duplicating rows, which is the worst failure this code can have. It now refuses with a message naming the limit. Also releases the cached connection and SDK result buffer on the new limit-reached branch, matching what FINAL_LOAD_TO_DUCKDB already does, so a MAX_ROWS read does not hold a SAP work process until query teardown. Suites: 47 C++ cases, RFC SQL 31/31 on nwrfc and 31/31 on proto.
Documents that threads parallelises across columns and therefore does nothing for a narrow extract -- the thing that sends people to raise threads and watch it change nothing -- and that partitions splits rows instead. States the measured curve rather than implying the worker count: 2.23s -> 0.83s at eight partitions on a 164,664-row single-column extract, with the knee visible so nobody sets it to 64 and expects more. Both caveats are stated plainly: rows come back unordered, which is why this is opt-in; and ROWSKIPS is a 32-bit integer, so a window cannot start past row 2.1 billion -- erpl refuses rather than wrapping, because a wrap would duplicate rows.
erpl_rfc_partition_window_rows is a UBIGINT the user sets, and the scheduler rounded it UP to a whole number of batches. Rounding a near-max value wraps, and a window_size of 0 makes Claim()'s fetch_add return the SAME offset to every worker forever -- every worker reads the same rows, so the scan returns duplicates rather than an error. Found by a third adversarial review round, after two earlier rounds had reviewed the design and the first implementation. The test that pins it fails with "0 > 0" on the old code. Now clamped to INT32_MAX before rounding -- nothing above that is reachable anyway, since ROWSKIPS is an ABAP INT4 -- and rounded DOWN, which cannot overflow while keeping every offset a multiple of the batch size. Also divides erpl_rfc_fetch_size across partition workers. The budget bounds (columns x batch) for ONE reader, and each worker owns its own state machines and SDK result buffers, so leaving it undivided would have made peak memory scale with the partition count. Measured on a 2.87M-row extract: 215 MB at one partition, 374 MB at eight -- 1.7x memory for 4.2x speed, rather than the 8x a per-worker budget would give. Narrow scans are unaffected: the batch hits MAX_BATCH_SIZE long before the divided budget binds.
TrySelectFallbackReadTableFunction switches read_table_function when a string column read fails with TABLE_WITHOUT_DATA, but read_table_result_path and read_table_import_params were probed against the PREVIOUS function at bind time and do not carry over. Left stale, the retry asks /SAPDS/RFC_READ_TABLE2 for "/DATA", ResolveResultTable finds nothing and returns 0 rows. On the unpartitioned path that is an empty result; on a partitioned scan a worker reads 0 rows as end-of-table and stops the whole scan. Either way it is silent truncation, which is the worst thing this code can do. Both are now recomputed for the newly selected function, inside the lock, before any other worker observes the switch. Found by a fourth review round. The first three had looked at the fallback for thread-safety and stopped there, having concluded the mutex settled it. Also makes RfcRowWindowScheduler total for any batch size rather than relying on its only caller staying well behaved -- a batch above INT32_MAX would previously have produced a window that could wrap the ROWSKIPS arithmetic. Not reachable through sap_read_table today, since the batch comes from MaxBatchSizeForColumnCount and is capped, but the class should not depend on that. 50 C++ cases, 3018 assertions.
A fifth review round took the round-4 fix as its subject and found three things. **The retry still used the old result path.** `data_path` was computed once before the retry loop, so after the fallback switched read_table_function and reprobed the path, the retry asked /SAPDS/RFC_READ_TABLE2 for RFC_READ_TABLE's "/DATA" anyway. Fixing the member without fixing the local left the original symptom exactly where it was: zero rows, which a partitioned worker reads as end-of-table. It is now re-read every attempt. **The readers raced the writer.** read_table_function, read_table_result_path and read_table_import_params were written under the fallback mutex and read without it. read_table_import_params is a std::set -- reading one while another thread clears and refills it is undefined behaviour, not a stale answer. A partitioned scan makes this reachable: several workers build RFC calls while one of them falls back. The accessors now take the same lock. The two Resolve* functions are safe to call while holding it because they read read_table_function directly rather than through the accessors. **Claim() could wrap.** next_offset.fetch_add was unchecked, so with enough claims the offset wraps and starts handing out ranges that have already been read. It now stops at the ROWSKIPS ceiling, which is where a window becomes unusable anyway. The test asserting two successive claims from an INT32_MAX-sized window was wrong and has been corrected: exactly one claim is possible there, and the second must be REFUSED rather than advancing past what ROWSKIPS can address. What the test really guards -- that no offset is ever handed out twice -- is now asserted directly. 50 C++ cases, 3003 assertions.
The gitlink pointed at 983ffd33 -- a commit with no tag ancestor, roughly twenty commits behind the current release, recorded back in August. Anything testing "the proto backend" was testing that, not a release. v2026.9.1 brings, among others: beca45e basXML selected and implemented end to end 0f7f1d3 two divergences a side-by-side against PyRFC 3.3.1 exposed 04c541b the 17 module-level names pyrfc was missing 2d82178 throughput: count what a client can honestly count basXML matters most here: the ODP path parses it directly. Pinning to a tag rather than a branch commit also stops the submodule drifting. A local checkout had wandered to v2026.9.1-5-g088a7e7 -- five commits PAST the release, on a branch -- and the build quietly reset it back to the recorded gitlink mid-run, so measurements taken either side of a build were against different implementations without saying so.
The newest release, v2026.9.1, breaks ODP replication: RODPS_REPL_ODP_FETCH_XML makes ABAP short-dump with RUNT_INTERNAL_ERROR in SAPLRODPS_REPL, and 14 of erpl's 24 ODP tests fail on it. Reported as DataZooDE/erpl-proto#53. Bisected across release tags to a single commit: v2026.8.29 6d8dfe1 ok v2026.8.29.1 0f7f1d3 ok <- pinned here v2026.8.30 beca45e BROKEN <- "basXML: ... implement it end to end" v2026.8.31 125b594 BROKEN v2026.9.1 4e50ef1 BROKEN RODPS_REPL_ODP_OPEN still succeeds on the broken releases -- the ODQ cursor is registered -- and sap_odp_preview still works, because it goes through a call that does not negotiate an XML format. It is format selection specifically that ABAP rejects. This is still a move forward: the previous gitlink was 983ffd33, sixteen commits BEFORE v2026.8.29 and carrying no tag at all. v2026.8.29.1 is a real release, is newer, and keeps ODP working. Move the pin to v2026.9.1 or later once #53 is fixed -- nothing else in that range is a problem for erpl, and the RFC suite is green on it.
They are unrelated to row-range partitioning and were swept in by an overbroad add. They stay on disk, untracked, as they are on master.
jrosskopf
changed the title
v2026.9.2 fixes the table-delta padding gap (#54) but regresses BICS from 45/45 to 23/45 on a nested-TTYP basXML defect (erpl-proto#60). One RFC test is the cheaper cost, so the pin stays and the gap stays recorded.
…e-partitioning # Conflicts: # CHANGELOG.md
Collaborator
Author
|
Merged So this PR now carries the last piece of the three-bug batch as well as row-range partitioning. Re-running CI before merge. |
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.
What this adds
sap_read_tablegains row-range partitioning:PARTITIONS := Nsplits a scaninto N disjoint
ROWSKIPS/ROWCOUNTwindows read concurrently, andFETCH_SIZE := Nexposes the per-batch row budget that until now was onlyreachable through
erpl_rfc_read_table_batch_budget.This closes the structural gap that column-parallelism left open: a narrow table
got no parallelism at all, because the existing scan parallelises across columns.
Correctness
Row order is not part of the contract, so every partitioned path is checked
against its serial reference with the symmetric
EXCEPT ALLmultiset oracle —order-insensitive, multiplicity-preserving, so it catches drops, duplicates and
value corruption in one assertion.
count(*)would pass a drop-one/duplicate-onebug; it is not used as the oracle anywhere here.
rfc/test/sql/sap_read_table_partitions.testruns that oracle across partitioncounts, and
rfc/test/cpp/test_row_window_scheduler.cppcovers the scheduleroffline, including an 8-thread no-gap/no-overlap case.
Fixes found while building it
Each of these was caught by measurement or by a review round, not by the tests I
wrote first:
STANDARD_VECTOR_SIZEinstead of the effective batch budget. Only thebenchmark caught it.
ROWSKIPScould wrap. It is an ABAP INT4; a large offset overflowed.MAX_ROWScombined with partitions left a state machine with work pending that it could
never complete, hanging the scan.
RFC_READ_TABLEfallback raced under concurrent partitions andleft stale metadata behind. Now serialised behind a lock; the first attempt at
this fix was itself incomplete (it updated the member but the retry used a
path captured before the loop).
Measurement, and what it is not
Partitioning helps, but the honest framing is in
API_REFERENCE.md: the gaindepends entirely on how much concurrency the target system sustains. The numbers
I could take are from a single-container trial and are not a statement about
what a production SAP system does — nothing here caps a default at a
trial-measured value.
Also
Pins erpl-proto to v2026.8.29.1. That is deliberately not the newest release:
v2026.9.1 carries two defects that break ODP and table deltas
(DataZooDE/erpl-proto#53, #54 — fixes now open as #55 and #56). Re-pin once those
land.