-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(low code): Fix missing cursor for ClientSideIncrementalRecordFilterDecorator #334
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new boolean flag, Changes
Sequence Diagram(s)sequenceDiagram
participant RS as RecordSelector
participant TR as Transformer
participant FL as Filter
alt Transformation Before Filtering (flag True)
RS ->> TR: Transform records
TR -->> RS: Return transformed records
RS ->> FL: Filter transformed records
FL -->> RS: Return filtered records
else Filtering Before Transformation (flag False)
RS ->> FL: Filter records
FL -->> RS: Return filtered records
RS ->> TR: Transform filtered records
TR -->> RS: Return transformed records
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/extractors/record_selector.py (1)
108-118
: Consider adding docstring to explain the transformation order behavior.The conditional logic is clear, but adding a docstring would help users understand the impact of
transform_before_filtering
on the processing order.def filter_and_transform( self, all_data: Iterable[Mapping[str, Any]], stream_state: StreamState, records_schema: Mapping[str, Any], stream_slice: Optional[StreamSlice] = None, next_page_token: Optional[Mapping[str, Any]] = None, ) -> Iterable[Record]: + """ + Process records by applying transformations and filtering. + + When transform_before_filtering is True, records are transformed before being filtered. + This is useful when the filter depends on transformed values. + Otherwise, records are filtered first and only matching records are transformed. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/extractors/record_selector.py
(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
(3 hunks)unit_tests/sources/declarative/extractors/test_record_selector.py
(2 hunks)unit_tests/sources/declarative/parsers/test_model_to_component_factory.py
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/extractors/record_selector.py (1)
44-44
: LGTM! Good default value choice.The
transform_before_filtering
flag is initialized toFalse
which maintains backward compatibility with existing behavior.unit_tests/sources/declarative/extractors/test_record_selector.py (1)
225-311
: LGTM! Comprehensive test coverage.The test is well-structured with:
- Clear test cases for both transformation orders
- Good documentation explaining the test scenarios
- Thorough assertions verifying the behavior
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
2419-2430
: LGTM! Clean integration with client-side incremental sync.The changes correctly set
transform_before_filtering
to True when client-side incremental sync is enabled, ensuring transformed values are available for filtering.
2445-2445
: LGTM! Proper inclusion in RecordSelector initialization.The
transform_before_filtering
flag is correctly passed to the RecordSelector constructor.unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)
1197-1197
: LGTM! The new assertions validate the transform_before_filtering behavior.The added assertions verify that transformations are applied before filtering when using client-side incremental sync, which is the expected behavior. This helps ensure data consistency and proper record processing order.
Also applies to: 1279-1279
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* main: fix: update cryptography package to latest version to address CVE (airbytehq#377) fix: (CDK) (HttpRequester) - Make the `HttpRequester.path` optional (airbytehq#370) feat: improved custom components handling (airbytehq#350) feat: add microseconds timestamp format (airbytehq#373) fix: Replace Unidecode with anyascii for permissive license (airbytehq#367) feat: add IncrementingCountCursor (airbytehq#346) feat: (low-code cdk) datetime format with milliseconds (airbytehq#369) fix: (CDK) (AsyncRetriever) - Improve UX on variable naming and interpolation (airbytehq#368) fix: (CDK) (AsyncRetriever) - Add the `request` and `response` to each `async` operations (airbytehq#356) fix: (CDK) (ConnectorBuilder) - Add `auxiliary requests` to slice; support `TestRead` for AsyncRetriever (part 1/2) (airbytehq#355) feat(concurrent perpartition cursor): Add parent state updates (airbytehq#343) fix: update csv parser for builder compatibility (airbytehq#364) feat(low-code cdk): add interpolation for limit field in Rate (airbytehq#353) feat(low-code cdk): add AbstractStreamFacade processing as concurrent streams in declarative source (airbytehq#347) fix: (CDK) (CsvParser) - Fix the `\\` escaping when passing the `delimiter` from Builder's UI (airbytehq#358) feat: expose `str_to_datetime` jinja macro (airbytehq#351) fix: update CDK migration for 6.34.0 (airbytehq#348) feat: Removes `stream_state` interpolation from CDK (airbytehq#320) fix(declarative): Pass `extra_fields` in `global_substream_cursor` (airbytehq#195) feat(concurrent perpartition cursor): Refactor ConcurrentPerPartitionCursor (airbytehq#331) feat(HttpMocker): adding support for PUT requests and bytes responses (airbytehq#342) chore: use certified source for manifest-only test (airbytehq#338) feat: check for request_option mapping conflicts in individual components (airbytehq#328) feat(file-based): sync file acl permissions and identities (airbytehq#260) fix: (CDK) (Connector Builder) - refactor the `MessageGrouper` > `TestRead` (airbytehq#332) fix(low code): Fix missing cursor for ClientSideIncrementalRecordFilterDecorator (airbytehq#334) feat(low-code): Add API Budget (airbytehq#314) chore(decoder): clean decoders and make csvdecoder available (airbytehq#326)
Resolves: #330
Summary by CodeRabbit
New Features
Tests