8000 support immediate query cancellation by pjain1 · Pull Request #2 · pjain1/go-duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

support immediate query cancellation #2

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
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

support immediate query cancellation #2

wants to merge 6 commits into from

Conversation

pjain1
Copy link
Owner
@pjain1 pjain1 commented Jan 10, 2023

No description provided.

begelundmuller
begelundmuller requested changes Jan 10, 2023
View reviewed changes
Copy link
Collaborator
@begelundmuller begelundmuller left a comment

Choose a reason for hiding this comment

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

Two general comments:

  1. Consider extracting the common steps in ExecContext and QueryContext into a helper function to avoid accidentally fixing something in only one and not the other
  2. Consider adding a comment before the query execution loop that a) explains it's for checking context cancellation, b) linking to relevant docs or implementation in other driver

@pjain1
Copy link
Owner Author
pjain1 commented Jan 10, 2023

@begelundmuller thanks for the comments. I was thinking of removing Query and Exec methods from connection and statement files altogether thats why did not remove the duplication. Not sure if someone might be directly using them, what do you think ?
8000

@begelundmuller
Copy link
Collaborator

@begelundmuller thanks for the comments. I was thinking of removing Query and Exec methods from connection and statement files altogether thats why did not remove the duplication. Not sure if someone might be directly using them, what do you think ?

I also thought about that. It's very likely fine to remove them, though they're also easy to keep (can just call the context equivalent using context.Background()). Maybe remove them to keep things clean?

Just to clarify, my comment above about duplication referred to the duplicated logic in ExecContext and QueryContext.

@pjain1
Copy link
Owner Author
pjain1 commented Jan 11, 2023 •
edited
Loading

though they're also easy to keep

Its not straightforward to call as xxxContext accepts driver.NamedValue where as regular methods accept driver.Value so will have to convert between these before calling the corresponding xxxContext methods.

Will clean up duplicated logic.

@pjain1
Copy link
Owner Author
pjain1 commented Jan 11, 2023

I can have something like this to convert between the two

func valueToNamedValue(values []driver.Value) ([]driver.NamedValue, error) {
	args := make([]driver.NamedValue, len(values))
	for n, param := range values {
		args[n].Value = param
		args[n].Ordinal = n + 1
	}
	return args, nil
}

@pjain1 pjain1 requested a review from begelundmuller January 11, 2023 09:47
begelundmuller
begelundmuller reviewed Jan 11, 2023
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@begelundmuller begelundmuller begelundmuller requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pjain1 @begelundmuller
0