feat(format): introduce ADBC API revision 1.1.0

[lidavidm]

Fixes #317.

[lidavidm]

I've addressed feedback + added APIs for most of the new proposals. If this is OK I'll follow up with Go (with a set of interfaces, one per method or so) and Java (which is very easy).

I think we will have to punt on #320 and #621. I don't think #630 is worth doing. #685 is too speculative at this point.

[zeroshade]

what would this be used for? is it just to avoid consumers having to call strtof/strtod themselves?

[lidavidm]

There's a constant above motivating it. Also, 1) there's no Get* function at all right now and 2) returning strings is really annoying due to allocations.

I could change this to return a string, the caveats would be either: 1) we have to add some release-callback thing (C Data Interface-ish) or 2) we return a const char*, but calling this again or doing other things with the statement will invalidate the pointer (SQLite-ish) or 3) we expect a caller-allocated and/or fixed-size buffer (ODBC-ish)

[zeroshade]

shouldn't this also have the same for the other new functions? AdbcStatementCancel and AdbcStatementGetDouble?

[lidavidm]

I'm going to defer implementation, just wanted to demonstrate the basic idea worked. I just want to get the API sort of in the right shape.

[lidavidm]

Agh, let me retarget this PR against the branch, not main.

[lidavidm]

There - the intent is to build this out on a branch then manually ff-merge it at the end. If the approach looks reasonable I'm also going to build out more serious compatibility tests.

[zeroshade]

Gotcha, okay then in that case I'm in favor of this approach.

[lidavidm]

Interestingly the CI failure exposes what does look like a compatibility issue: the new fields in AdbcDriver make the Python test crash (because it mixes a driver using the new definition and a wheel using the old definition)

[lidavidm]

Ah, because the drivers do this

  auto* driver = reinterpret_cast<struct AdbcDriver*>(raw_driver);
  std::memset(driver, 0, sizeof(*driver));

and now that the struct has changed, they need to actually check the size of the struct they're about to manipulate.

[lidavidm]

I'm quite tempted to just remove cpplint, it seems to mostly just give noise and it's basically unconfigurable/unmaintained

[lidavidm]

Some other thoughts: maybe we shouldn't overload getInfo for things like current catalog/schema, and instead add a get/set option pair? That would be consistent with how Java does this

[lidavidm]

Added Java and Go, though I think I'd like to go back and align things a bit.

[lidavidm]

There's a macOS error in CI that I'll try to track down, but otherwise I think this is ready (for C, Java, Go)

[lidavidm]

TODOs

• Reserve more space for AdbcDriver?
• Figure out macOS CI error (doesn't replicate under Rosetta...)

[lidavidm]

I never really learned amd64 assembly but I think the CI failure I'm seeing is possibly a miscompilation or something odd? I see basically an unconditional call to UBSAN's abort here:

    0x103770820 <+64>:   movq   -0x10(%rbp), %rax
    0x103770824 <+68>:   movq   %rax, -0x20(%rbp)
    0x103770828 <+72>:   movq   -0x20(%rbp), %rax
    0x10377082c <+76>:   movq   %rax, -0x28(%rbp)
    0x103770830 <+80>:   xorl   %eax, %eax
    0x103770832 <+82>:   testb  $0x1, %al
    0x103770834 <+84>:   jne    0x10377084a               ; <+106> at postgresql.cc
    0x10377083a <+90>:   leaq   0x109f3f(%rip), %rdi      ; _dyld_private + 158976
    0x103770841 <+97>:   xorl   %eax, %eax
    0x103770843 <+99>:   movl   %eax, %esi
    0x103770845 <+101>:  callq  0x1037f75ae               ; symbol stub for: __ubsan_handle_type_mismatch_v1_abort

aka (if I'm reading it right):

• -0x10(%rbp) is raw_driver,
• 4 instructions of shuffling registers/stack,
• a comparison that always fails (since we zero out EAX, then bitwise-AND it with 1 before doing a conditional jump),
• an unconditional call to __ubsan_handle_type_mismatch_v1_abort

Ghidra seems to agree:

  if (param_1 == 1000000) {
    if (param_2 == 0) {
      local_9 = 5;
    }
    else {
      ___ubsan_handle_type_mismatch_v1_abort(&DAT_00194780,0);

I'm not sure why this is happening, or if there's UB in the code and this is a consequence of it...

In the debugger, I can confirm raw_driver is not NULL despite UBSAN reporting a null-pointer-dereference.

[lidavidm]

For now let's see if writing it differently helps...

[lidavidm]

Ah, I think I know what happened.

[pitrou]

Should the pointer-to-function be checked for null? Or is it the caller's job?

[lidavidm]

It's assumed that if you're using these functions, you're using the driver manager, and that the driver manager has initialized all the functions. (Obviously, not true right now, since I decided to split implementation out.)

[lidavidm]

I'm going to merge this soon (into the branch) and then update/continue the follow-up PRs.

[lidavidm]

Ah...let me file a follow up PR for some of Antoine's last comments

[lidavidm]

#731