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

query cancellation attempt #383

Merged
merged 3 commits into from
Apr 22, 2024
Merged

Conversation

hrl20
Copy link
Contributor

@hrl20 hrl20 commented Apr 17, 2024

Implement query cancellation for DuckDB/MotherDuck using conn.interrupt() duckdb/duckdb#7895

@hrl20 hrl20 force-pushed the louisa/query-cancellation branch 2 times, most recently from 5d7d1d5 to f0a374b Compare April 17, 2024 21:02
@hrl20 hrl20 force-pushed the louisa/query-cancellation branch from f0a374b to 14ce3a2 Compare April 17, 2024 21:02
@@ -51,10 +51,6 @@ class DuckDBAdapter(SQLAdapter):
def date_function(cls) -> str:
return "now()"

@classmethod
def is_cancelable(cls) -> bool:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm should we make this environment dependent, possibly?

@@ -58,6 +58,9 @@ def notify_closed(self):
if self.handle_count == 0 and not self._keep_open:
self.close()

def cancel(cls, connection: Connection):
connection.handle.cursor().interrupt()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a new feature of MD or of DuckDB in general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a duckdb feature: duckdb/duckdb#7895
I also manually tested it with MD and saw the expected behavior.

@hrl20 hrl20 marked this pull request as ready for review April 19, 2024 19:17
@hrl20 hrl20 requested a review from jwills April 22, 2024 15:24
Copy link
Collaborator

@jwills jwills left a comment

Choose a reason for hiding this comment

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

Looks great @hrl20, thank you so much!

@jwills
Copy link
Collaborator

jwills commented Apr 22, 2024

The failing tests are the flaky attach test-- I'm writing a PR to make it non-flaky

@jwills jwills merged commit 44a5468 into duckdb:master Apr 22, 2024
27 of 31 checks passed
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.

None yet

2 participants