DMS: Fix handling of primary keys #104
Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
""" WalkthroughThe changes update AWS DMS to CrateDB translation by adding explicit handling of primary key columns as OBJECT(STRICT) with type resolution. The Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Translator as DMSTranslatorCrateDBRecord
participant UniversalRecord as UniversalRecord
Test->>Translator: to_sql(MSG_CONTROL_CREATE_TABLE)
Translator->>Translator: pk_clause()
Translator->>Translator: resolve_type() (for each PK column)
Translator-->>Test: CREATE TABLE ... with PK, typed, untyped columns
Test->>Translator: to_sql(MSG_LOAD or MSG_INSERT)
Translator->>Translator: decode_record(record)
Translator->>UniversalRecord: from_record(record, primary_keys)
UniversalRecord-->>Translator: UniversalRecord instance
Translator-->>Test: INSERT INTO ... (pk, data, aux) ON CONFLICT DO NOTHING
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
force-pushed
the
fix-dms-pk
branch
2 times, most recently
from
2d28683 to
f4ea9fd
Compare
amotl
force-pushed
the
fix-dms-pk
branch
3 times, most recently
from
578cffe to
06590ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commons_codec/transform/aws_dms.py (1)
86-89: Guard against duplicate PKs when processing repeatedcreate-tableevents
self.primary_keys.extend(pks)runs only once, butpkscan still contain duplicates which would later producedata['id']=:id AND data['id']=:id …clauses.- self.primary_keys.extend(pks) + for pk in pks: + if pk not in self.primary_keys: + self.primary_keys.append(pk)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGES.md(1 hunks)src/commons_codec/model.py(2 hunks)src/commons_codec/transform/aws_dms.py(8 hunks)src/commons_codec/transform/dynamodb.py(1 hunks)tests/transform/test_aws_dms.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGES.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/commons_codec/transform/dynamodb.py (1)
src/commons_codec/model.py (2)
UniversalRecord(137-164)from_record(151-164)
🪛 GitHub Check: codecov/patch
src/commons_codec/transform/dynamodb.py
[warning] 136-136: src/commons_codec/transform/dynamodb.py#L136
Added line #L136 was not covered by tests
src/commons_codec/model.py
[warning] 161-161: src/commons_codec/model.py#L161
Added line #L161 was not covered by tests
🔇 Additional comments (2)
src/commons_codec/transform/dynamodb.py (1)
136-138: Nice simplification – leverages the newUniversalRecord.from_record()The refactor removes hand-rolled splitting logic and delegates to the shared helper, improving consistency across translators.
No issues spotted here.tests/transform/test_aws_dms.py (1)
230-253: Tests clearly cover both PK and non-PK insert pathsGood job extending coverage – both branches of
insertlogic are now exercised.
... by inheriting fragments of the translation logic from the DynamoDB CDC handler.
Problem
With DMS, the SQL DDL statement didn't include the primary key definition.
References