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.

Sign up for GitHub

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

Jump to bottom

Add try/catch to cursor.fetchall() #7504

Closed
wants to merge 2 commits into from
Closed

Add try/catch to cursor.fetchall() #7504

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5c5c8b
Add try/catch to cursor.fetchall()
leiwan5 May 14, 2019
9eff28b
Update core.py
leiwan5 May 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion superset/models/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,10 @@ def _log_query(sql):
for sql in sqls[:-1]:
_log_query(sql)
self.db_engine_spec.execute(cursor, sql)
cursor.fetchall()
try:
cursor.fetchall()
except BaseException:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I might be missing something obvious here, but why are we calling fetchall() if we're not even storing the return value anywhere? Wouldn't it be enough to just call exexute() here and remove the fetch all together?

Copy link
Author

Choose a reason for hiding this comment

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

I was confused about here too. We only want data from the last query.

Copy link
Member

Choose a reason for hiding this comment

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

Aah yes, it's explained here: #5517. I think this looks good, but it might be good to add a comment explaining that some engines require calling fetchall() to force the execute() calls to execute synchronously.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like the raw connection exposes the underlying DB-API. Per PEP 249 the fetchall method raises an Error and thus sense we should use the SQLAlchemy equivalent exception here rather than BaseException.


_log_query(sqls[-1])
self.db_engine_spec.execute(cursor, sqls[-1])
Expand Down