fix: Stop dataset iterators from skipping items when unwind is used - #1059
Open
vdusek wants to merge 1 commit into
Open
fix: Stop dataset iterators from skipping items when unwind is used#1059vdusek wants to merge 1 commit into
vdusek wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1059 +/- ##
==========================================
- Coverage 95.24% 92.32% -2.92%
==========================================
Files 59 59
Lines 5509 5514 +5
==========================================
- Hits 5247 5091 -156
- Misses 262 423 +161
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
What was wrong
iterate_itemsadvanced the dataset offset bymax(scanned_rows, len(items)). Withunwind, a page returns moreitems than the rows it scanned, so the offset jumped past rows the next request never read. At the default
chunk_size, a 5000-row dataset with a 3x unwind yielded about a third of it.The fix
The offset now advances by at most the number of rows the call asked for. The items endpoint applies
offsetandlimitto the rows first and shapes the result afterwards, and it passes nomaxLimit, so a page never covers morerows than the
limitit was sent. Capping the advance there keeps an unwound page from running past the window itactually read. A page covering fewer rows than requested has hit the end of the dataset, so an overshoot on that last
page costs nothing.
The
max()stays underneath the cap on purpose.x-apify-pagination-countcomes from the dataset'sitemCount,which the API increments through a throttled write on a 5s interval, while the items themselves are served fresh.
Following the header alone, which is what apify-client-js#1044
does and what #1058 suggested, would truncate
iterate_itemsright afterpush_itemsfor anything over one page.The JS iterator bounds itself by
totaland so is exposed to that lag either way; this one consults neither, whichis why the two clients end up with different fixes.
DatasetItemsPage.countkeeps its current value, so the decision #1058 asked for turned out to be moot. Only itsdocstring changes, along with the
limitdocs oniterate_items: both promised items where the field and theparameter have always counted scanned rows.
Tests
unwindreached the pagination tests for the first time. The fake API now splits each row into three items afterthe row window is picked, exactly as the transform stream does, and three cases iterate through it.
endpoints. It now applies the requested limit verbatim on the items endpoint, so a
chunk_sizeabove that cap isfinally covered.
countlags behind its items has a test of its own. Nothing pinned that before, and it is theinvariant the
max()rests on.One thing found while verifying
get_cursor_iteratorjustified its termination rule with filters that "can drop every item on a page while a livecursor still points at more data". The API rules that out: the key-value store returns the last key of the page as
the next cursor, and the request queue returns a cursor only once a page came back full, so an empty page carries no
cursor. The behavior is fine and unchanged; the docstring now says why it holds.
Closes #1058
✍️ Drafted by Claude Code