-
Notifications
You must be signed in to change notification settings - Fork 95
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
feat: ADBC API revision 1.1.0 #971
Conversation
It looks like I added #442 to the header but did not implement it, so I'll go do that. Otherwise, this should be ready for review, though I'll also take some time to review the implementation. |
Hmm, it looks like the Linux packages all still have at most Go 1.18. That's a bit of a problem, since then we can't propagate our cancellation error... |
Alright, I'll revert to Go 1.18 (though, leaving Go 1.20 in CI mostly) and instead of context.Cause rely on exact comparison with context.Canceled and context.DeadlineExceeded |
e65e69e
to
7f79f2b
Compare
@zeroshade this is ready for review. I'll be posting to the ML this week for voting unless I hear objections. |
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.
This is very exciting! I'm particularly excited for cancellation and progress options since these are frequently requested features for general database access in R that have long been unresolved.
I think this has come up in some other discussion; however, just a note for the record that I'm looking forward to implementing the R bindings on top of this! Since the R bindings are a (very) thin wrapper around the C header, I think it makes sense to vote on this without R and I will implement the required changes for the first ADBC libraries release that implements it 🙂 . |
I think the Snowflake test failures are the two test runs colliding with each other. |
.github/workflows/native-unix.yml
Outdated
go-version-file: 'go/adbc/go.mod' | ||
go-version: 1.20.7 |
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.
any particular reason to specify the version explicitly instead of pull it from the go.mod
?
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.
I've been through this already in prior PRs, I'm not really interested in redoing all this. (+ it's annoying trying to use an old Go since it's tied to the version of golangci-lint.)
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.
Reverted, though I'll have to figure out staticcheck and golangci-lint again...
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.
we can just bump the go.mod to be 1.20
and use the newer golangci-lint and staticcheck
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.
We can't, because debian etc. only have go 1.18
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.
again, I had to go through this all in the previous PR 😬
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.
Most of everything has already been reviewed before it was merged into this branch in the first place. Did a quick look over everything and nothing stands out as problematic or as having had an issue during the merge. so it all LGTM 😄
Thanks for all the work here @lidavidm I know it wasn't easy coordinating all this.
445d8be
to
ca4f151
Compare
I realized I forgot about #417 so I'll grab that, assume we're OK with declaring that parameter |
Fixes #317. --------- Co-authored-by: Matt Topol <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]>
- Clarify one of the docstrings - Remove the 'future growth' padding since we have a different compatibility strategy
Filed #995 for the error in the Documentation pipeline |
Fixes #55.
Fixes #317.
Fixes #318.
Fixes #319.
Fixes #442.
Fixes #458.
Fixes #459.
Fixes #541.
Fixes #620.
Fixes #685.
Fixes #736.
Fixes #755.
Fixes #939.
Fixes #940.
Fixes #942.
Fixes #962.