Skip to content
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

Support writes in mysql #1935

Closed
wants to merge 12 commits into from
Closed

Support writes in mysql #1935

wants to merge 12 commits into from

Conversation

mfouilleul
Copy link
Contributor

Avoid trigging errors during insert, update, deletes.

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Please see comments.

columns = self.fetch_columns([(i[0], types_map.get(i[1], None)) for i in cursor.description])
rows = [dict(zip((c['name'] for c in columns), row)) for row in data]

queries = query.rstrip(';').split(';')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not such a good idea, think of the following (contrived) example:

>>> "SELECT ';'".split(';')
["SELECT '", "'"]

But see the fix here: e02fdb3 for supporting multiple queries.

Once you rebase with latest, you can simplify the logic you implemented here.

error = None

if transaction:
connection.commit()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not commit above?

else:
transaction = True
columns = [{'name': 'Row(s) Affected',
'type': types_map.get(str(type(rows_count)).upper(), None)}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be always an integer? In which case you can use TYPE_INTEGER constant.

@mfouilleul
Copy link
Contributor Author

Do we need more changes for this?

@arikfr
Copy link
Member

arikfr commented May 13, 2019

Is this still needed following the changes in #3003?

@Jakdaw
Copy link
Contributor

Jakdaw commented May 15, 2019

I'm not sure how much need there is for writes from redash - with the #3003 changes you can now achieve the same thing; eg with a redash query like:-

UPDATE mytable SET a=a+1 WHERE b={{whatever}};
SELECT ROW_COUNT() as "Row(s) Affected";

@arikfr
Copy link
Member

arikfr commented Aug 11, 2019

Following #3003, closing this. @mfouilleul, if you still think this is needed (and willing to put in the work to update it to the changes in redash.query_runner.mysql) -- let me know. I will review it again before you do some extra work.

Thanks.

@arikfr arikfr closed this Aug 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants