fix(dlt): compare dlt version semantically for destination client#5890
Conversation
StuffbyYuki
left a comment
There was a problem hiding this comment.
Thanks for this!
Given destination_client() exists on all realistically supported dlt versions and is what everyone already hits due to the string-compare bug, would you consider simplifying to always use destination_client() (or pinning dlt>=1.10) instead of maintaining a version gate?
Let me know what you think
71733d8 to
899bafd
Compare
|
Thanks for the suggestion. I simplified this to always use I also rebased on latest
The failing |
|
@anxkhn Can you rebase your branch? there was a fix that got merged that fixes the test-dbt-version ci test |
The dlt destination-client selection gated the 1.10.0 API change with a string comparison (`dlt.__version__ >= "1.10.0"`). String comparison is lexicographic, so it wrongly evaluated "1.9.0" >= "1.10.0" (and "1.2.0", "1.5.10", ...) as True, sending every 1.x version down the >=1.10.0 branch. `destination_client()` is the public accessor on every realistically supported dlt version, and the pre-1.10 `_sql_job_client` fallback was already unreachable because of the same string-compare bug, so drop the version gate and always use `destination_client()`. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
899bafd to
57678e4
Compare
The dlt destination-client selection in
sqlmesh/integrations/dlt.pygated the1.10.0 API change with a string comparison:
String comparison is lexicographic, not semantic, so it is wrong for any version
with a double-digit minor. For example
"1.9.0" >= "1.10.0"evaluates toTrue(the character
'9'sorts after'1'), and likewise for"1.2.0"and"1.5.10". The result is that every1.xversion takes the>= 1.10.0branch,and the pre-1.10
_sql_job_clientfallback (added in #4223 for compatibilitywith older dlt) is unreachable.
This is currently latent rather than crashing, because dlt versions below 1.10.0
happen to expose
destination_client()as well. But the version guard issilently incorrect and the intended fallback never runs.
The fix uses
packaging.version.parsefor a semantic comparison, mirroring theidiom already used in
sqlmesh/core/config/connection.py(
version.parse(...) < version.parse("...")).packagingis already atop-level dependency, so no new dependency is introduced.
The client selection is extracted into a small
_get_destination_clienthelperso the version gate can be unit tested without a live dlt pipeline.
Test Plan
test_get_destination_client_version_gateintests/integrations/test_dlt.py: a parametrized, pure-Python test thatmonkeypatches
dlt.__version__and asserts1.9.0,1.2.0,1.5.10select_sql_job_clientwhile1.10.0,1.28.1,2.0.0selectdestination_client. It fails under the old string comparison for the pre-1.10cases and passes after the fix. No warehouse or live dlt pipeline required.
pytest tests/integrations/test_dlt.py-> 7 passed.make style(ruff check, ruff format, mypy) clean.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO