fix: prevent record filters from prematurely stopping PageIncrement pagination#1073
Conversation
…top pagination early Co-Authored-By: Daryna Ishchenko <darina.ishchenko17@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1784038497-page-increment-record-filter#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1784038497-page-increment-record-filterPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesPage pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Requester
participant PageIncrement
participant RecordExtractor
Requester->>PageIncrement: pass HTTP response
PageIncrement->>RecordExtractor: extract records when configured
RecordExtractor-->>PageIncrement: return extracted records
PageIncrement-->>Requester: return next page token or None
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
PageIncrementstops paginating whenlast_page_size < page_size, butSimpleRetriever._read_pagescomputeslast_page_sizefrom records after theRecordSelectorruns (post record-filter). So a record filter (e.g. client-side incremental filtering) that drops records from a full API page causes pagination to stop early — and if an entire page is filtered out, the sync stops entirely even though more pages exist.OffsetIncrementwas already fixed for this exact issue by accepting an optionalextractorthat re-counts the raw (pre-filter) records from the response. This PR mirrors that approach forPageIncrement:PageIncrementgains an optionalextractor: Optional[RecordExtractor]field; when set,next_page_tokenrecomputeslast_page_size = len(extractor.extract_records(response))before applying the stop condition.ModelToComponentFactory.create_page_incrementnow builds that extractor from the stream'srecord_selector.extractormodel (same pattern ascreate_offset_increment, using theextractor_modelalready threaded throughcreate_default_paginator).Behavior is unchanged for direct
PageIncrementinstantiations without an extractor.Requested by Daryna Ishchenko (Daryna Ishchenko (@darynaishchenko)).
Link to Devin session: https://app.devin.ai/sessions/ef2cfea26843460dafd703d1d4adaf29
Summary by CodeRabbit
Bug Fixes
Tests