feat(go/adbc/driver/databricks): Add Databricks driver written in Go #3325
Conversation
Ingesting Jade's PR for Databricks Go Driver
jasonlin45
changed the title
jasonlin45
changed the title
felipecrv
left a comment
felipecrv
left a comment
There was a problem hiding this comment.
I still need to review statement.go and driver.go.
| // Transaction methods (Databricks has limited transaction support) | ||
| func (c *connectionImpl) Commit(ctx context.Context) error { | ||
| // Databricks SQL doesn't support explicit transactions in the traditional sense. | ||
| // Most operations are auto-committed. We'll track state but not perform any operation. |
There was a problem hiding this comment.
| // Most operations are auto-committed. We'll track state but not perform any operation. | |
| // Most operations are auto-committed. |
felipecrv
left a comment
felipecrv
left a comment
There was a problem hiding this comment.
@zeroshade can you give a quick look to this? I'm sure we will find bugs when we start using it, but we will surely follow up with fixes as we identify problems.
|
@lidavidm Thanks for the in depth review! I just applied the following changes:
|
| if d.sslMode != "" { | ||
| switch strings.ToLower(d.sslMode) { | ||
| case "insecure": | ||
| if tlsConfig == nil { | ||
| tlsConfig = &tls.Config{MinVersion: tls.VersionTLS12} | ||
| } | ||
| tlsConfig.InsecureSkipVerify = true | ||
| case "require": | ||
| // Default behavior - full TLS verification | ||
| if tlsConfig == nil { | ||
| tlsConfig = &tls.Config{MinVersion: tls.VersionTLS12} | ||
| } | ||
| default: | ||
| return nil, adbc.Error{ | ||
| Code: adbc.StatusInvalidArgument, | ||
| Msg: fmt.Sprintf("invalid SSL mode: %s (supported: 'require', 'insecure')", d.sslMode), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
same as above, move this to SetOptions instead of here
| // TODO: Prepared statement support with raw connections | ||
| if s.prepared != nil { | ||
| return nil, -1, adbc.Error{ | ||
| Code: adbc.StatusNotImplemented, | ||
| Msg: "Prepared statements are not yet supported via `execute query`", | ||
| } | ||
| } |
There was a problem hiding this comment.
We shouldn't require a prepared statement here. If we have one, then use it, otherwise use s.query.
There was a problem hiding this comment.
use the same logic as at line 172
There was a problem hiding this comment.
I don't believe we are?
if s.prepared != nil
This errors if a prepared statement DOES exist, otherwise it uses the set query
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
staticcheck/pre-commit need to be satisfied.
| github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.4.0 // indirect | ||
| github.com/BurntSushi/toml v1.4.0 // indirect | ||
| github.com/andybalholm/brotli v1.2.0 // indirect | ||
| github.com/apache/arrow/go/v12 v12.0.1 // indirect |
There was a problem hiding this comment.
...Does databricks not use the latest arrow-go? 🙁
Any chance that can be fixed?
There was a problem hiding this comment.
Great callout - I just went to the latest dependencies on databricks-sql-go and it looks to be v12.0.1 for latest v1.8.0.
There was a problem hiding this comment.
@jadewang-db perhaps this can be updated on the SDK side for upcoming v1.9.0?
| defer func() { | ||
| if closeErr := rows.Close(); closeErr != nil { | ||
| err = errors.Join(err, closeErr) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The return value still needs to be named.
|
@lidavidm Please take a look again when you get the chance! pre-commit should be green now |
|
I'm going to merge it based on the 2 approvals and the fact that the CI failure is unrelated. |
felipecrv
changed the title
jasonlin45
changed the title
5 participants
This PR is a continuation of #2998
Key changes from Jade's PR:
Connection Pooling
Databricks offers a
sql.DBstruct which manages a connection pool. This is now initialized on the ADBCDatabasestruct when connections are opened and re-used if no options have changed on the Database.Connections can be obtained from the pool which are stored on the ADBC Connection.