feat: add NestedRecordExtractor for extracting nested collections with parent context - #1164
Anatolii Yatsuk (tolik0) wants to merge 2 commits into
Conversation
…h parent context A DpathExtractor that reaches a child collection has already discarded the node that held it, so records needing a field from their parent cannot be produced declaratively. NestedRecordExtractor keeps the parent in scope: for every record its parent_extractor yields it reads child_field_path out of that record, yields each element, and copies the configured parent_fields onto it first. parent_extractor accepts DpathExtractor, CustomRecordExtractor, or another NestedRecordExtractor. Extraction stays lazy: nothing is materialised into a list and the response is read exactly once, by the innermost extractor. The generated model class was hand-applied rather than regenerated, and verified byte-identical against the output of `poe assemble`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
👋 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@tolik0/cdk/nested-record-extractor#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 tolik0/cdk/nested-record-extractorPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
Replace the `dpath` calls in the component with explicit path walks. That is what resolves most of these findings: `dpath` reads globs out of literal segments, refuses to overwrite a non-object intermediate, and turns a numeric segment into a list, none of which this component wants. * Read each `parent_field` once per parent instead of once per child. The read is invariant across the children, and because it walked a parent that contains the child collection the extractor was quadratic in the size of that collection: 100 children per parent cost 807 us/record, now 4.0. * Copy mutable values per child. The parent's value was shared by reference, so a downstream `AddFields` writing into it rewrote records that had already been emitted — they are queued unserialized. Scalars are still shared. * Replace a `record_path` intermediate that is present but is not an object, so the documented "a key already present on the child is overwritten" holds for a nested `record_path` too. It previously raised `PathNotFound` or `TypeError` mid-sync, surfaced as a `system_error` naming no manifest field. * Reject the `*` wildcard, which the schema already documented as unsupported, at parse time and after interpolation. It used to resolve when a body had one match and raise when the next page had two. `?` and `[...]` now address the fields that spell them. * Accept a `Mapping` that is not a `MutableMapping`. The `RecordExtractor` contract is `Mapping` and the schema admits a `CustomRecordExtractor` as a `parent_extractor`, so such a parent silently produced an empty stream. * Count parents, not children, in `OffsetIncrement` and `PageIncrement`. The three `extractor_model` unions did not admit the new model, and the strategies advance by the number of records the response held, so the offset over-advanced and records were skipped. * Reject a `parent_path` that addresses the child collection or an ancestor of it, and a segment that interpolated to nothing. * Raise rather than copy `None` when a `parent_path` runs into a value holding no such field, which was indistinguishable from the documented absent-key case on a field used as a primary key or cursor. A null still copies `None`. * Register `NestedRecordExtractor.parent_extractor` in `CUSTOM_COMPONENTS_MAPPING`, so a custom extractor declared with only a `class_name` keeps working when wrapped. * Export `ParentFieldPath` and register `ParentFieldPathModel`; log every skip path; narrow the mutable-state claim in the docstring; scope the comment about not copying the child to the child dict. Also document that `DpathExtractor` with a `record_expander` plus `AddFields` already covers the single-level case, and the three shapes it cannot express, which is why this component exists. The schema changes are descriptions and `parent_fields` examples only. No `pattern` is added: it would emit `constr(regex=...)`, the construct MyPy rejects for the same reason as `conint`. The three edited blocks in the generated models were re-checked byte-for-byte against pinned `datamodel-code-generator==0.26.3` output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Closing in favour of #1165. While reviewing this PR together with #1149, #1162 and #1163 against the source-github migration, it turned out that The check that settles it: both What this PR genuinely added over Shipping this as a parallel component would have left two ways to write "explode this list and stamp the parent onto each child" with no way for a Connector Builder user to choose between them, so it is closed rather than merged. |
1 participant
What
Adds a new declarative component,
NestedRecordExtractor, which yields the records of a collection nested inside another record while carrying fields down from the record that contains it.For every record the
parent_extractoryields,child_field_pathis read out of that record and each element is emitted, with eachparent_fieldsentry copied from the parent onto the element first. Both paths are lists, so a value nested inside the parent can be copied into a nested position on the child; intermediate objects onrecord_pathare created as needed. Paths are interpolated againstconfigand$parameters.Changes:
airbyte_cdk/sources/declarative/extractors/nested_record_extractor.py— new, withNestedRecordExtractorand theParentFieldPathcopy instruction.airbyte_cdk.sources.declarative.extractorsand from the top-levelairbyte_cdkpackage, followingDpathExtractor.declarative_component_schema.yaml— theNestedRecordExtractorandParentFieldPathdefinitions, plusNestedRecordExtractoradded to theRecordSelector.extractoranyOf.model_to_component_factory.py—create_nested_record_extractor,create_parent_field_path, and thePYDANTIC_MODEL_TO_CONSTRUCTORentry.unit_tests/sources/declarative/extractors/test_nested_record_extractor.py(34 tests) and four factory tests.Why
A
DpathExtractorpointed at a child collection has already discarded the node that held it, so a record that needs a field from its parent cannot be produced declaratively at all. Connectors work around this with a customcomponents.py, which makes the connector ineligible for the Connector Builder and for manifest-only packaging.Two verified instances, both on the unmerged branch
tolik0/source-github/graphql-streamsinairbytehq/airbyte, fileairbyte-integrations/connectors/source-github/source_github/components.py:NestedGraphQLRecordExtractor, lines 291–337. Walksdata.repository.<list_connection>.nodes[*]— or, for the drill-down document,data.repository.<drilldown_field>— descends into each parent's<child_connection>.nodes, and copies parent fields onto every child.reviews.pull_request_urlcomes from the pull request'surl.DeepNestedGraphQLRecordExtractor, lines 490–543. The same idea one level deeper: reactions are stamped withcomment_idtaken from the comment they hang off.Both shapes are reproduced end to end in the tests against realistic response bodies — the
reviewslisting document and drill-down document, andpull_request_comment_reactionsfrom the repository-listing root and from each of the threenoderoots.What this does and does not buy
It removes those two extractor classes from source-github. It does not make source-github manifest-only or Builder-eligible: the two GraphQL pagination strategies and
ReleasesRecordTransformationstay custom, so the connector keeps itscomponents.py. A previous PR in this area claimed source-github as a motivating connector in a stronger sense than was true and had to retract that during review; this is the narrow, accurate version of the claim.Decisions
A
parent_paththat is absent on the parent copiesNoneMatches the source-github code, which does
parent.get(source). Skipping the field is equally defensible, but copyingNonekeeps the record shape stable across the stream instead of making a field's presence depend on the parent — which matters downstream, where a missing column and a null column are not the same thing to destination normalization. Tests:test_missing_parent_path_copies_none,test_missing_nested_parent_path_copies_none.An existing key on the child is overwritten
Also matches source-github. The parent is the authoritative source for the fields the manifest explicitly names, and silently keeping the child's value would hide a misconfigured
record_path. Test:test_existing_key_on_child_is_overwritten.parent_extractormay itself be aNestedRecordExtractorAllowed. This is the widening beyond
DpathExtractor/CustomRecordExtractor, and the reason is concrete: it is the only way to copy fields from two different ancestor levels onto one record. ADpathExtractorwith*wildcards can flattenpullRequests.nodes.*.reviews.nodes.*.comments.nodes.*, but flattening discards every intermediate node, so the flattened form can never stamp bothpull_request_urlandreview_id. Nesting costs nothing to support — aNestedRecordExtractoralready satisfiesRecordExtractorand stays lazy — so this is a schema line, not code. Tests:test_nested_parent_extractor(a comment carrying bothreview_idand the grandparent'spull_request_id),test_create_nested_record_extractor_nested_in_itself.The
AsyncRetrieverandFileUploaderextractor unions are deliberately left alone; there is no demonstrated need and each one widens the contract.Config-time rejection
child_field_pathmust be non-empty. An empty path would make the child the parent itself — a self-stamping identity extractor, which is never what was meant. Enforced twice:minItems: 1in the schema, which rejects it during manifest validation before any record is read (test_create_nested_record_extractor_rejects_empty_child_field_path), and a guard in__post_init__for anyone constructing the component directly (test_empty_child_field_path_is_rejected).ParentFieldPathrejects an emptyparent_pathorrecord_paththe same way.Other documented behaviour, each pinned by a test
test_children_are_mutated_in_placeso nobody "fixes" it into a copy without noticing they changed the contract.DpathExtractoralready wraps one into a single-element list. The source-github drill-down document depends on this, and the samechild_field_paththerefore serves both documents.child_field_paththat is missing, null, or resolves to an empty collection yields nothing rather than raising, matching howDpathExtractortreats a path that does not resolve. An empty object yields nothing for the same reason.child_field_pathdoes not support the*wildcard. Flattening across several collections belongs in theparent_extractor; the last hop has to stay a single collection for its parentage to be well defined — anddpath.valueswould build a list, breaking laziness.Laziness and concurrency
Nothing is materialised:
_childrenyields straight out of the listdpath.getreturns, andextract_recordsis a generator throughout.test_is_lazy_and_does_not_materialise_childrenuses a counting stub parent extractor and asserts that exactly one parent has been pulled after the first record is produced. The response is read exactly once, by the innermost extractor —test_response_is_read_oncepins that.CombinedExtractor(#1162) documents why re-reading matters: a streaming decoder is drained by the first reader and the loss is silent. There is only one sub-extractor here, so that is a constraint on this code, not a guard it needs.The component holds no mutable state — the only per-read values are locals in
extract_records— because one instance is shared by every partition of a stream and partitions are read concurrently.test_no_mutable_state_on_the_componentinterleaves two reads from one instance and asserts they do not see each other.One test pinning a specific risk
test_typename_dispatch_equivalence_is_query_dependent. When the four reaction roots are expressed as separate paths, the__typenamedispatch inDeepNestedGraphQLRecordExtractorlooks redundant, because aPullRequestReviewhas noreviewsfield. That equivalence holds only because the drill-down documents do not selectPullRequest.comments, which is a real GraphQL connection. The test builds a response where aPullRequestnode carries bothreviewsandcommentsand asserts the purely structural, path-based behaviour, so a future reader knows the equivalence is query-dependent rather than structural, and that selectingPullRequest.commentsmeans adding a root.Generated model
declarative_component_schema.pyis generated, but the output ofpoe assembleis not committed here. The pinneddatamodel-code-generator==0.26.3invocation inbin/generate_component_manifest_files.pyomits--field-constraints, so every numericminimum/exclusiveMinimumin the YAML is emitted asconint(...)/confloat(...), which mypy rejects withInvalid type comment or annotation [valid-type]. The committed file is also behind the YAML in other ways, so a full regeneration produces a large unrelated diff plus a red MyPy Check made mostly of errors that are not from this PR.So the
ParentFieldPathandNestedRecordExtractormodel classes, the widenedRecordSelector.extractorunion, and theNestedRecordExtractor.update_forward_refs()call were hand-written into the committed file in codegen style, then verified:poe assemblewas run,ruff formatapplied to its output, and the two class blocks diffed against the regenerated ones. Both are byte-identical, the union line and theupdate_forward_refs()line each appear exactly once in both, and everything else the generator produced was discarded.Breaking change: NON_BREAKING
Verified rather than asserted. What I checked:
RecordSelector.extractoranyOf. A union that gains a member accepts strictly more manifests than before; no existing definition, required-field list, or property type changed.AsyncRetrieverandFileUploaderare untouched.RecordSelector.extractorunion.DpathExtractorremains the first member, so an existingDpathExtractormanifest still validates against it under pydantic v1's left-to-right union resolution, and aNestedRecordExtractordict cannot be mistaken for it becausetypeis aLiteral.NestedRecordExtractor,ParentFieldPath); nothing was removed, renamed, or re-typed.__init__.pyexport lists andmanifest_component_transformer.py's"RecordSelector.extractor": "DpathExtractor"default, which is unchanged — an omittedtypeonRecordSelector.extractorstill defaults toDpathExtractorexactly as before. I deliberately did not add aNestedRecordExtractor.parent_extractordefault type: the field accepts three types including itself, so an explicittypeis the unambiguous thing to require.poetry run pytest unit_tests/sources/declarativepasses 1833 tests with no changes to any existing test, other than the four added factory tests and the import line they need.#1162(CombinedExtractor) has not merged tomain, soNestedRecordExtractorwas not added to theCombinedExtractor.extractorsanyOfand nothing here depends on that branch.Verification
🤖 Generated with Claude Code