feat(go/adbc/driver/flightsql): add support for flightsql:// uri format#4488
feat(go/adbc/driver/flightsql): add support for flightsql:// uri format#4488Mandukhai-Alimaa wants to merge 3 commits into
Conversation
Mandukhai-Alimaa
requested review from
lidavidm,
paleolimbot and
zeroshade
as code owners
xborder
left a comment
xborder
left a comment
There was a problem hiding this comment.
I'm probably missing something. This is changing the documentation but there is no implementation, is it intended?
| Connect over gRPC with TLS (secure). This is the default when no | ||
| ``transport`` query parameter is given. | ||
|
|
||
| ``flightsql://<host>:<port>?transport=tls`` |
There was a problem hiding this comment.
Would it make sense to use something similar to useEncryption=true/false like in flightsql jdbc driver? Is there something similar on another ADBC driver?
I'd suggest flightsql://<host>:<port> to default to grpc+tls and maybe allow useEncryption/ssl=true|false like other APIs do. Is there a similar example in ADBC that we can replicate?
There was a problem hiding this comment.
The proposed format mirrors this driver's own legacy grpc+tcp/grpc+tls/grpc+unix schemes, which is the more immediate precedent for someone migrating from the old URI format in this driver.
There was a problem hiding this comment.
Aligning with Flight SQL JDBC seems reasonable, though - what does it do for domain sockets?
| ``transport=unix``, or a socket path with ``transport=tcp`` or the | ||
| default TLS transport, is rejected as an invalid combination. | ||
|
|
||
| .. note:: For backwards compatibility, the driver also still accepts |
There was a problem hiding this comment.
If we're still allowing legacy schemes for backwards compatibility, what is the benefit of having transport=tcp|tls|unix?
|
|
||
| func main() { | ||
| db, err := sql.Open("flightsql", "uri=grpc://localhost:12345") | ||
| db, err := sql.Open("flightsql", "uri=flightsql://localhost:12345?transport=tcp") |
There was a problem hiding this comment.
does the driver actually support this query param like that?
There was a problem hiding this comment.
This is a proposal on what the URI might look like. I will make another commit for the implementation soon
Mandukhai-Alimaa
changed the title
4 participants
This PR adds support for
flightsql://uri formatCloses #4460.