fix(rfc): refuse the ROWSKIPS ceiling instead of truncating, and bound the fallback retry - #139
Merged
Merged
Conversation
…d the fallback retry
Two defects in the partitioned scan path, both found by a multi-agent review of the
day's diff and confirmed against the code.
1. Claim() reported the ABAP INT4 ceiling by returning false -- the same answer it
gives when a table is exhausted. RfcReadTableScanPartitioned treats that as normal
completion and retires the worker with an empty chunk, and DuckDB reads an empty
chunk as end-of-scan. A scan past 2,147,483,647 rows therefore returned a truncated
prefix and called it success, while API_REFERENCE says erpl refuses rather than
wrapping.
This guard was added to stop next_offset wrapping and handing out already-read
offsets -- duplicated rows. Collapsing it into "exhausted" traded that for missing
rows, which is no better and is harder to notice. Claim now returns
CLAIMED / EXHAUSTED / ADDRESS_LIMIT and the scan raises on the last.
The bound was also off by one: `claimed > INT32_MAX - window_size` refused the last
legal window start (2,147,450,880 at a 32768 window). next_offset is 64-bit so
subtracting the window bought no wrap protection, and every individual ROWSKIPS
inside that window is already range-checked loudly in CreateFunctionArguments. An
existing test asserted the old behaviour ("exactly one claim is possible") and had
encoded the off-by-one; it now asserts the corrected contract.
2. The TABLE_WITHOUT_DATA fallback branch retried with `continue` without incrementing
the attempt counter and without sleeping. TrySelectFallbackReadTableFunction returns
a cached success once the switch has happened, so a second TABLE_WITHOUT_DATA from
the fallback function looped forever with no delay, once per partition worker,
against a shared SAP system. It now retries only if the selected function actually
changed; otherwise the original error propagates.
Offline: partition scheduler 13/13, batching 6/6.
Live: RFC 32/32 on nwrfc and 32/32 on erpl-proto.
…n-address-limit # Conflicts: # CHANGELOG.md
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
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.
Two defects in the partitioned scan path, both found by a six-persona review crew over the
day's diff and each verified against the code before being fixed.
1. The
ROWSKIPSceiling returned a truncated result instead of refusingRfcRowWindowScheduler::Claim()signalled the ABAPINT4limit by returningfalse— thesame answer it gives when a table is exhausted.
RfcReadTableScanPartitionedtreats thatas normal completion and retires the worker with an empty chunk, and DuckDB reads an
empty chunk as end-of-scan. So a scan past 2,147,483,647 rows returned a truncated prefix
and called it success, while
API_REFERENCEstates that erpl refuses rather than wrapping.The guard exists to stop
next_offsetwrapping and handing out already-read offsets —duplicated rows. Collapsing it into "exhausted" traded that for missing rows, which is
no better and is harder to notice. This is the same silent-wrong-results class as the two
defects fixed earlier in this scan path, and I introduced it.
Claimnow returnsCLAIMED / EXHAUSTED / ADDRESS_LIMIT; the scan raises on the last,naming the limit and what to do about it.
Off-by-one in the same guard.
claimed > INT32_MAX - window_sizerefused the lastlegal window (start 2,147,450,880 at a 32768-row window).
next_offsetis 64-bit, sosubtracting the window bought no wrap protection, and every individual
ROWSKIPSinsidethat window is already range-checked loudly in
CreateFunctionArguments. The bound is nowclaimed > INT32_MAX.An existing test asserted the old behaviour ("exactly one claim is possible") — it had
encoded the off-by-one, and now asserts the corrected contract: two starts are legal, the
third is refused.
2. The
RFC_READ_TABLEfallback could retry without limitIn the
TABLE_WITHOUT_DATAbranch the retry usedcontinuewithout incrementingattemptand without sleeping.
TrySelectFallbackReadTableFunctionreturns a cached success oncethe switch has happened, so a second
TABLE_WITHOUT_DATAfrom the fallback function was anunbounded tight loop against the SAP system with no delay — multiplied by the partition
count. Switching is one-way, so a repeat is a real failure.
It now retries only when the selected function actually changed; otherwise the original
error propagates.
Verification
Not fixed here
The review's other high finding — partitioned scans invalidate the RFC connection at every
window and exhaust the 16-slot persistent cache, so the default configuration performs a
logon per call — is a real performance defect but wants its own change and its own
before/after logon-rate measurement, not a ride-along in a correctness PR.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.