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

feat(csharp): Initial changes for ADBC 1.1 in C# implementation #1821

Merged
merged 14 commits into from
May 10, 2024

Conversation

CurtHagenlocher
Copy link
Contributor

This adds all the basic elements defined since the initial ADBC 1.0 specification. There's still a bunch of cleanup likely to be required, including support for exporting an ADBC 1.1 driver. There's also no test coverage (boo! hiss!) as the DuckDB driver does not yet implement any 1.1 features and some ci infrastructure work is probably needed to support other drivers.

Closes #1215

@CurtHagenlocher CurtHagenlocher requested a review from lidavidm as a code owner May 5, 2024 15:32
@github-actions github-actions bot added this to the ADBC Libraries 1.0.0 milestone May 5, 2024
@lidavidm lidavidm removed this from the ADBC Libraries 12 milestone May 7, 2024
@CurtHagenlocher
Copy link
Contributor Author

@davidhcoe I'd appreciate your opinion on this change; at your leisure, of course.

@davidhcoe
Copy link
Contributor

Overall, this is a good step toward a 1.1 implementation. I am not sure how we got the BulkIngest method in AdbcConnection, since the spec doesn't define that, but Java has it too, so we will need to clean that up at some point.

The other part I don't know about is whether having SetOption(string, object) is sufficient to having the SetOptionByte, SetOptionInt, etc., methods that are defined in the spec. I see they are defined in the C importer/exporter, though, just not in the core C# definitions. I think this is largely because having different methods means having different dictionaries as well, which would seem strange to have to go look through different dictionaries to find where a value is.

@CurtHagenlocher
Copy link
Contributor Author

If we want to pass all the initial options in at once when we construct something, then they kind of have to be represented as "object". Post construction, it might be justified to have separate overloads for setting different types but I also feel like this is an area where it's reasonable to want to support both e.g. SetOption("adbc.x", "true") and SetOption("adbc.x", true) for a bool-valued option that's officially represented as a string. And if we're going to convert anyway... .

@CurtHagenlocher CurtHagenlocher merged commit df6b340 into apache:main May 10, 2024
6 checks passed
@CurtHagenlocher CurtHagenlocher deleted the dev/curth/csharp-1.1.0 branch May 10, 2024 02:01
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.

feat(csharp): implement ADBC 1.1 specification
3 participants