fix(destination-motherduck): handle camel cased object/array columns - #83694
Conversation
|
Note 📝 PR Converted to Draft More info...Thank you for creating this PR. As a policy to protect our engineers' time, Airbyte requires all PRs to be created first in draft status. Your PR has been automatically converted to draft status in respect for this policy. As soon as your PR is ready for formal review, you can proceed to convert the PR to "ready for review" status by clicking the "Ready for review" button at the bottom of the PR page. To skip draft status in future PRs, please include |
👋 Welcome to Airbyte!Thank you for your contribution from Donnype/airbyte! We're excited to have you in the Airbyte community. If you have any questions, feel free to ask in the PR comments or join our Slack community. 💡 Show Tips and TricksPR Slash CommandsAs needed or by request, Airbyte Maintainers can execute the following slash commands on your PR:
Tips for Working with CI
📚 Show Repo GuidanceHelpful Resources
|
|
↪️ Triggering Reason: The PR is ready for review and has no AI review on record yet, so a review pass is the next pipeline gate. |
Reviewing PR for connector safety and quality.
|
🛡️ AI PR Review Report🔴 Review Action: REQUEST CHANGES
🔶 Risk Level: 3/5Logic change in the destination write path (column-name normalization in 🔧 Remediation RequiredCI Checks (FAIL) — Unit tests all passed ( Live / E2E Tests (UNKNOWN) — this is a bug fix touching sync-write behavior, so validation is required, but no 📋 PR DetailsConnector(s): 🔍 Gate Evaluation DetailsGate-by-Gate Analysis
Change under review: 📚 Evidence ConsultedEvidence
❓ How to RespondResolving a Failing GateEach non-PASS gate above lists concrete remediation. Apply it, push, then re-run Written explanations do not change a gate verdict. If you believe a gate is a false positive or it cannot be remediated as written, request review from a human maintainer and explain the situation to them in a PR comment — a human reviewer can approve or merge over a bot FAIL. Never edit the PR description to argue with the bot; the description is how reviewers learn what the PR does. |
|
🙋 Escalated to #human-in-the-loop per Hands-Free AI Triage Project triage next step. Reason: |
|
/run-connector-tests
|
There was a problem hiding this comment.
Pull request overview
Fixes a MotherDuck destination edge case where JSON column serialization is skipped when source schema property names are camelCase/PascalCase but buffer keys are normalized (lowercased), which can reintroduce DuckDB “empty STRUCT” failures.
Changes:
- Normalize JSON schema property keys before deciding whether to JSON-serialize buffered columns.
- Extend unit/integration tests to cover PascalCase/UpperCase JSON array/object columns.
- Bump
destination-motherduckversion to0.2.6and document the release.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/integrations/destinations/motherduck.md | Adds release note entry for 0.2.6 describing the fix. |
| airbyte-integrations/connectors/destination-motherduck/unit_tests/test_serialize_object_columns.py | Adds regression coverage for normalized vs source-property name mismatch. |
| airbyte-integrations/connectors/destination-motherduck/pyproject.toml | Bumps connector version to 0.2.6. |
| airbyte-integrations/connectors/destination-motherduck/metadata.yaml | Updates Docker image tag to 0.2.6. |
| airbyte-integrations/connectors/destination-motherduck/integration_tests/integration_test.py | Adds an end-to-end regression for an UpperCase array-of-objects JSON field. |
| airbyte-integrations/connectors/destination-motherduck/destination_motherduck/processors/duckdb.py | Normalizes schema property names before JSON-column serialization decisions. |
Suppressed comments (2)
airbyte-integrations/connectors/destination-motherduck/unit_tests/test_serialize_object_columns.py:63
LowerCaseNormalizershould be instantiated before being passed into_serialize_object_columns(it’s used asLowerCaseNormalizer()in other tests).
buffer_data = {"id": ["1"], "array_of_objects": [[{}]]}
serialized = _serialize_object_columns(buffer_data, JSON_SCHEMA, LowerCaseNormalizer)
pa_table = pa.Table.from_pydict(serialized)
airbyte-integrations/connectors/destination-motherduck/unit_tests/test_serialize_object_columns.py:85
LowerCaseNormalizershould be instantiated before being passed into_serialize_object_columns(it’s used asLowerCaseNormalizer()in other tests).
buffer_data = {"id": ["1"], "line": [[{"Amount": 100.0, "SubTotalLineDetail": {}}]]}
serialized = _serialize_object_columns(buffer_data, JSON_SCHEMA, LowerCaseNormalizer)
pa_table = pa.Table.from_pydict(serialized)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _serialize_object_columns( | ||
| buffer_data: Dict[str, List[Any]], | ||
| json_schema: dict, | ||
| normalizer: type[NameNormalizerBase], | ||
| ) -> Dict[str, List[Any]]: |
There was a problem hiding this comment.
IIRC, I think this class is designed to work either way - with class methods and optional instance properties.
It may be fine as-is, or optionally as:
| def _serialize_object_columns( | |
| buffer_data: Dict[str, List[Any]], | |
| json_schema: dict, | |
| normalizer: type[NameNormalizerBase], | |
| ) -> Dict[str, List[Any]]: | |
| def _serialize_object_columns( | |
| buffer_data: Dict[str, List[Any]], | |
| json_schema: dict, | |
| normalizer: type[NameNormalizerBase] | NameNormalizerBase, | |
| ) -> Dict[str, List[Any]]: |
| def test_serialize_object_columns(col_name, values, expected) -> None: | ||
| result = _serialize_object_columns({col_name: values}, JSON_SCHEMA) | ||
| result = _serialize_object_columns({col_name: values}, JSON_SCHEMA, LowerCaseNormalizer) | ||
| assert result[col_name] == expected |
There was a problem hiding this comment.
Ditto my last comment. Passing the class is correct and expected.
|
/ai-prove-fix
|
✅ Fix Proven —
|
Airbyte Support Bot (airbyte-support-bot)
added
the
hyd-prove
|
c3bbd19
into
airbytehq:master
github-project-automation
Bot
moved this from New PRs
to Done
in 🧑🏭 Community Pull Requests
What
The empty-struct bug and upper-cased columns bug joined forces and produced a new bug: upper-cased-empty-struct-columns are broken. After #82244 this became more apparent since now also arrays pass that logic. This PR applies the same normalisation logic to that edge-case handling.
How
Pass the normalizer and apply the normalization to the JSON keys (column names) before moving forward.
Review guide
It's mostly test code, the real logic is the dict-comprehension changing the keys.
User Impact
The sync is failing for more complicated APIs that happen to use camel/pascalCase. This is blocking users from using Airbyte for ingest.
Can this PR be safely reverted and rolled back?