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(go/adbc/drivermgr): Implement Remaining CGO Wrapper Methods that are Supported by SQLite Driver #1304

Merged
merged 9 commits into from
Nov 21, 2023

Conversation

joellubi
Copy link
Member

@joellubi joellubi commented Nov 16, 2023

What?

Implementations for the following methods in the CGO wrapper for adbc_driver_manager:

  • GetTableSchema
  • GetTableTypes
  • Commit
  • Rollback
  • GetParameterSchema
  • BindStream

Why?

Functionality exists in C++ driver manager but not yet accessible via Go driver interface.

Notes

Three methods in the wrapper remain unimplemented: ExecutePartitions, ReadPartition, and SetSubstraitPlan. These methods are not currently supported by the SQLite driver, which is the primary test target for these changes. It is still possible to implement them in the drivermgr wrapper without support in specific drivers, but it does make it more difficult to verify correct behavior. The effort to add those methods will likely involve some additional work to ensure we are able to test their behaviors, so they are being left out of this current round of implementations.

Closes part of: #1291

@@ -186,6 +190,19 @@ func getRdr(out *C.struct_ArrowArrayStream) (array.RecordReader, error) {
return rdr.(array.RecordReader), nil
}

func getSchema(out *C.struct_ArrowSchema) (*arrow.Schema, error) {
// Maybe: ImportCArrowSchema should perform this check?
Copy link
Member Author

Choose a reason for hiding this comment

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

This check is needed for when catalog or dbSchema is specified for GetTableSchema. The SQLite driver does not return a schema when either of these are set. As a result, we attempt to import a schema through the cdata interface with all nil fields including format which panics when it reaches this line.

It does seem like it would be helpful to perform these nil checks in the cdata package, but I just wanted to check first as that would require cross-repo changes and add to the scope of these changes.

expSchema := arrow.NewSchema(
[]arrow.Field{
{Name: "id", Type: arrow.PrimitiveTypes.Int64, Nullable: true},
{Name: "name", Type: arrow.PrimitiveTypes.Int64, Nullable: true}, // Should be arrow.BinaryTypes.String
Copy link
Member Author

Choose a reason for hiding this comment

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

I would have expected the SQLite type TEXT to map to arrow.BinaryTypes.String. I've confirmed via the SQLite driver separately (see screenshot) that this is in fact the behavior of the current driver implementation, so this test actually does indicate the correct behavior for the CGO wrapper.

image

Copy link
Member

Choose a reason for hiding this comment

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

SQLite doesn't have types. So the only thing the driver can do is try to read some rows and infer an appropriate column type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @lidavidm, I didn't realize that. This now works as expected after inserting test data.

@joellubi joellubi marked this pull request as ready for review November 16, 2023 12:57
@lidavidm lidavidm added this to the ADBC Libraries 0.9.0 milestone Nov 16, 2023
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

This LGTM.

@lidavidm any further comments or issues on this?

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Only gave it a quick look but LGTM.

@zeroshade zeroshade merged commit cc0c4e9 into apache:main Nov 21, 2023
28 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.

3 participants