fix(c/driver_manager): incorrect buffer length check in AdbcDatabaseGetOption #3797
Conversation
amoeba
changed the title
|
Hey @lidavidm, I saw this while debugging something else and I wasn't sure if I was understanding it right so I thought I'd put up a PR with a failing test. Does that conditional look backward? |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Yeah, that conditional seems backwards indeed...
amoeba
force-pushed
the
fix/AdbcDatabaseGetOption-buf-size-check
branch
from
61941cd to
9018e05
Compare
|
I pushed up a change to the conditional and rebased to pick up a fix for an unrelated CI failure. |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
FWIW, do the postgres/sqlite drivers make the same mistake?
|
postgresql was good, I wasn't sure about sqlite. |
|
It looks like the Go drivers may have the same issue, I'll open a fresh PR for that. |
|
I looked at postgres and the base framework (which sqlite delegates to) and they both have the correct behavior. The Go drivers look to have the wrong logic, I'll file a follow-up PR when I confirm. |
|
Update: The go driver implementation looks fine so we don't need a follow-up PR. |
2 participants
This test covers the behavior of AdbcDatabaseGetOption with out different out buffer lengths and exposes what I think is a bug in
arrow-adbc/c/driver_manager/adbc_driver_manager.cc
Line 1655 in 8ed6379