Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
49dd807 to
190f1af
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
+ Coverage 54.33% 54.36% +0.03%
==========================================
Files 203 203
Lines 9574 9574
==========================================
+ Hits 5202 5205 +3
+ Misses 4372 4369 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| "pytest-cov<8", | ||
| "pytest-xdist[psutil]<4", | ||
| "testcontainers[mysql,postgres]>=4.9.1,<4.15", | ||
| "testcontainers[cratedb,mysql,postgres] @ git+https://github.com/testcontainers/testcontainers-python.git@main", |
There was a problem hiding this comment.
Thank you very much for adding a native CrateDB adapter to Testcontainers for Python. With testcontainers 4.15, we will be able to use it in downstream packages swiftly.
| "testcontainers[cratedb,mysql,postgres] @ git+https://github.com/testcontainers/testcontainers-python.git@main", | |
| "testcontainers[cratedb,mysql,postgres]>=4.15,<4.16", |
| "dataclasses-json<0.7", | ||
| "dlt<1.29", | ||
| "dlt-cratedb<0.2", | ||
| "dlt-cratedb @ git+https://github.com/zerotired/dlt-cratedb.git@fix-dynamic-install", |
There was a problem hiding this comment.
@florinutz: It looks like even when using the newest updates in the dlt-cratedb repository, including yours, the venerable DestinationSchemaTampered error is still present across the board.
| if dest_uri.startswith("cratedb://"): | ||
| pytest.skip( | ||
| "Fails on CrateDB with `DestinationSchemaTampered`, see " | ||
| "https://github.com/crate/dlt-cratedb/issues/14" | ||
| ) | ||
|
|
There was a problem hiding this comment.
@florinutz: It looks like even when using the newest updates in the dlt-cratedb repository, including yours, the venerable DestinationSchemaTampered error is still present across the board.
Like demonstrated here, we had to skip all test cases that are still failing on CrateDB. It would be so sweet to resolve them in one way or another, so CrateDB can become a premium-grade data warehouse within omniload and other applications using dlt. 🙏
amotl
left a comment
There was a problem hiding this comment.
Let me flag two more errors.
| if dest_uri.startswith("cratedb://"): | ||
| pytest.skip( | ||
| 'Fails on CrateDB with `"_id" conflicts with system column`, ' | ||
| "see https://github.com/crate/dlt-cratedb/issues/19" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Currently we need to skip the MongoDB -> CrateDB integration tests, because of the venerable “_id” conflicts with system column pattern error that apparently still happens.
/cc @florinutz
| def append_test(dest_uri, dynamodb, schema: str): | ||
|
|
||
| if dest_uri.startswith("cratedb://"): | ||
| pytest.skip( | ||
| "Fails on CrateDB with twice the amount of expected results: AssertionError: assert 6 == 3" | ||
| ) | ||
|
|
There was a problem hiding this comment.
That is another type of error we haven't seen before with dlt and CrateDB. It needs to be investigated.
| assert res[0] == ("message1",) | ||
| assert res[1] == ("message2",) | ||
| assert res[2] == ("message3",) | ||
| if dest_uri.startswith("cratedb://"): | ||
| messages_db = [res[0][0], res[1][0], res[2][0]] | ||
| assert "message1" in messages_db | ||
| assert "message2" in messages_db | ||
| assert "message3" in messages_db | ||
| else: | ||
| assert res[0] == ("message1",) | ||
| assert res[1] == ("message2",) | ||
| assert res[2] == ("message3",) |
There was a problem hiding this comment.
Note to self: Maybe adjust the database query instead to emit ordered results?
kimberly-WY
left a comment
There was a problem hiding this comment.
Good improvement overall. One edge case: empty string vs null aren't treated the same downstream — might need a normalization step.
|
The Reproduced against CrateDB 6.3.0 nightly with a minimal dlt pipeline (timestamp + primary key):
Root cause is exactly #51: the released 0.1.1 (2026-03-10) predates crate/dlt-cratedb#49 / crate/dlt-cratedb#51, so But a release won't turn all five skips green. A 2-run merge on Suggest: after the release, drop the skips on any case that only needs the load to complete, and re-point the rest from crate/dlt-cratedb#14 (fixed) to crate/dlt-cratedb#6 (the real open item) as |
|
@hampsterx: Thanks for the detailed report about why and where dlt-cratedb might need further improvements. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="crate/dlt-cratedb"
for endpoint in \
"repos/$repo/issues/6" \
"repos/$repo/issues/14" \
"repos/$repo/pulls/49" \
"repos/$repo/pulls/51" \
"repos/$repo/releases?per_page=10"
do
echo "===== $endpoint ====="
gh api "$endpoint" --jq '
if type == "array" then
.[] | {tag_name, name, published_at, prerelease, draft}
else
{number, title, state, merged_at, closed_at, html_url, body}
end
' || true
doneLength of output: 7160
🐇 ✅ |
|
Tip For best results, initiate chat on the files or code changes.
Once the first item is released, this PR can unskip load-completion cases. The stateful merge assertions should remain 🐇👍 |
|
Tip For best results, initiate chat on the files or code changes.
After the release, this PR can unskip tests that only require successful loads. Tests asserting preservation of existing primary-key rows should remain 🐇👍 |
About
CrateDB is an emerging open-source data warehouse that can store data in the terabyte ranges and is largely compatible with PostgreSQL.
Details
An earlier attempt to validate CrateDB caused too much noise, because the database didn't support the
DATEtype. Now that this has changed recently, warehouse operations become actually viable.@DHRUV6029: Thank you very much for submitting the relevant patch to CrateDB. 👍
Status
The adjusted test suite currently needs to skip a few test cases that raise the venerable
DestinationSchemaTamperedexception of dlt. We think dlt-cratedb might need further adjustments beyond the patch recently submitted by @florinutz.pytest --no-cov -k cratedb 87 passed, 71 skipped, 54 warnings in 124.39s (0:02:04)DestinationSchemaTamperederrors withmergeanddelete+insertstrategies orrefresh="drop_resources": Schema content was changed by a loader or by destination code crate/dlt-cratedb#14merge,delete-insert, andrefresh="drop_resources"crashes crate/dlt-cratedb#51References