-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
include primary_key for upsert to avoid ValueError #143
base: main
Are you sure you want to change the base?
Conversation
When syncing a single column table from dolt to mysql, sqlalchemy will throw a ValueError complaining about an empty update_dict CREATE TABLE `authority` ( `name` varchar(50) NOT NULL, PRIMARY KEY (`name`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8; https://github.com/zzzeek/sqlalchemy/blob/857adaaf867df54d4a023cf19f618fdf1d0f60c9/lib/sqlalchemy/dialects/mysql/dml.py#L150
Codecov Report
@@ Coverage Diff @@
## master #143 +/- ##
===========================================
- Coverage 55.06% 41.69% -13.38%
===========================================
Files 25 23 -2
Lines 1718 957 -761
===========================================
- Hits 946 399 -547
+ Misses 772 558 -214
Continue to review full report at Codecov.
|
Hi @max-hoffman , I don't know how to increase the code coverage yet. Please kindly advise. |
The diff coverage would probably look OK if you merged master, a one line change wouldn't have that big of an effect. |
@@ -27,6 +27,6 @@ def get_target_writer(engine: Engine, update_on_duplicate: bool = True) -> DoltA | |||
|
|||
def upsert_helper(table: Table, data: List[dict]): | |||
insert_statement = insert(table).values(data) | |||
update_dict = {el.name: el for el in insert_statement.inserted if not el.primary_key} | |||
update_dict = {el.name: el for el in insert_statement.inserted} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @oscarbatori would be able to comment on the original code -- is there any unexpected behavior when we drop this primary key filter? Should we instead have a short circuit return if update_dict is empty? I have not spent a lot of time with the SQL-sync code since joining, can look at this more tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice. I tried a short circuit return, but the code I wrote was not clean enough, and that lead to this one line change.
When syncing a single column table from dolt to mysql,
sqlalchemy will throw a ValueError complaining about an empty update_dict
CREATE TABLE
authority
(name
varchar(50) NOT NULL,PRIMARY KEY (
name
)) ENGINE=InnoDB DEFAULT
CHARSET=utf8;
https://github.com/zzzeek/sqlalchemy/blob/857adaaf867df54d4a023cf19f618fdf1d0f60c9/lib/sqlalchemy/dialects/mysql/dml.py#L150