allow merge on expressions #388
Conversation
Deploying datachain-documentation with
|
| Latest commit: |
04a425b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c74f4386.datachain-documentation.pages.dev |
| Branch Preview URL: | https://impl-299.datachain-documentation.pages.dev |
| col: sqlalchemy.ColumnClause = ( | ||
| sqlalchemy.column(column) | ||
| if isinstance(column, str) | ||
| else sqlalchemy.column(column.name, column.type) |
There was a problem hiding this comment.
[F] It does not seem possible to overwrite the table of a column which already has the table set. I.e test_merge_with_itself_column fails without this change.
There was a problem hiding this comment.
This change is no longer required for this PR to work. I'd be happy to split it out if required.
There was a problem hiding this comment.
As for me personally I am OK to leave it here. It feels natural to have this change here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #388 +/- ##
==========================================
+ Coverage 87.28% 87.31% +0.02%
==========================================
Files 92 92
Lines 9960 9981 +21
Branches 2033 2041 +8
==========================================
+ Hits 8694 8715 +21
+ Misses 912 911 -1
- Partials 354 355 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
mattseddon
force-pushed
the
impl-299
branch
2 times, most recently
from
dfcda52 to
6b31c8a
Compare
mattseddon
changed the title
mattseddon
force-pushed
the
impl-299
branch
2 times, most recently
from
5b86816 to
da07321
Compare
| if "." in name: | ||
| name_path = name.split(".") | ||
| elif DEFAULT_DELIMITER in name: | ||
| name_path = name.split(DEFAULT_DELIMITER) |
There was a problem hiding this comment.
Just wonder, if it is possible to have both dot (.) and DEFAULT_DELIMITER (__) in column name? 🤔
There was a problem hiding this comment.
I don't think so. Seems like one is always swapped for the other.
| *right_on | ||
| ).db_signals() # type: ignore[assignment] | ||
|
|
||
| if len(right_on_columns) != len(on_columns): |
There was a problem hiding this comment.
Are we loosing this check after these changes or I miss something? 🤔
There was a problem hiding this comment.
The check is moved down. If any of the columns fail to resolve then we will have an entry in errors and we'll raise a DatasetMergeError.
| col: sqlalchemy.ColumnClause = ( | ||
| sqlalchemy.column(column) | ||
| if isinstance(column, str) | ||
| else sqlalchemy.column(column.name, column.type) |
There was a problem hiding this comment.
As for me personally I am OK to leave it here. It feels natural to have this change here.
Closes #299
This PR allows users to merge DataChains using
Columns and/or expressions (e.g.path.file_stem(dc.c("source.path"))). Examples of the new syntax are shown inexamples/multimodal/clip_inference.py&examples/multimodal/wds.py. Once this change has been merged I will make the appropriate changes in thedatachain-examplesrepo.It should be noted that the change is backwards compatible and
mergewill continue to accept strings for theonandright_onparameters.