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

[Format][C][Java] Consolidate ExecuteQuery/ExecuteUpdate #95

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

lidavidm
Copy link
Member

In C: combine ExecuteQuery/ExecuteUpdate since they're redundant

In Java: rename some things to be consistent with C. However, do not consolidate ExecuteQuery/ExecuteUpdate since the lack of out parameters means we don't have a way to differentiate the cases.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@@ -1061,7 +1048,7 @@ typedef AdbcStatusCode (*AdbcDriverInitFunc)(size_t count, struct AdbcDriver* dr
struct AdbcError* error);

// For use with count
#define ADBC_VERSION_0_0_1 27
#define ADBC_VERSION_0_0_1 26
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - the intent of the parameter is for the caller to express how many entries there are in the AdbcDriver struct, so the driver can fill it appropriately. And so this has to be updated when we add/remove items (well, until we vote, after which we would have to add a new #define for future entries).

It's sort of similar to what libraries like UCX do with field_mask here: https://openucx.github.io/ucx/api/latest/html/group___u_c_p___w_o_r_k_e_r.html#structucp__worker__params the caller/callee cooperate so that even if the struct size changes due to updates, they can still remain compatible

That said I've been debating whether we want to actually use this, or potentially do something like

typedef AdbcStatusCode (*AdbcDriverInitFunc)(size_t version, void* driver,
                                             size_t* initialized,
                                             struct AdbcError* error)

#define ADBC_VERSION_1_0_0 0x100
// if version == ADBC_VERSION_1_0_0, void* driver should point to a struct AdbcDriverV1
struct AdbcDriverV1 {
  // ... 
};

#deifne ADBC_VERSION_2_0_0 0x200
// if version == ADBC_VERSION_2_0_0, void* driver should point to a struct AdbcDriverV2
struct AdbcDriverV2 {
  struct AdbcDriverV1 base;
  // new functions... 
};

which might be easier to grok

Copy link
Member

Choose a reason for hiding this comment

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

I see.
Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

If you have opinions on the approach, I'd be interested!

Compatibility is also why most things are free functions, which is opposite of the C Data Interface…though for convenience, some structs do expose fields to the user

Copy link
Member

Choose a reason for hiding this comment

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

OK. I prefer the unique number approach to the number of members approach for version value.

Because we can support the following case with the unique number approach:

  1. An existing callback's signature is changed (e.g. add one more parameter). (The number of members isn't changed.)
  2. Remove a callback and add a new callback. (The number of members isn't changed.)

But we can avoid these cases by not changing released AdbcDriver's members. For example, we can just add a new member and deprecate an existing member for changing signature.

So the number of members approach will work (can keep backward compatibility) too.

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 think we will want to avoid changing a signature in either case to avoid causing ABI/compatibility issues.

I will write this up more clearly in #96. I think I'll sketch out the unique number approach because I think it is easier to understand, as well, and if so I'll switch things in that PR.

Thanks!

@lidavidm lidavidm merged commit 62581c5 into apache:main Aug 31, 2022
@lidavidm lidavidm deleted the lidavidm/query-execute branch August 31, 2022 13:15
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.

AdbcStatementExecuteUpdate/AdbcStatementExecuteQuery & rows_affected
2 participants