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] Document API/ABI stability goals #96

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

lidavidm
Copy link
Member

Fixes #77.

@lidavidm
Copy link
Member Author

lidavidm commented Aug 31, 2022

Rendered (current approach; this is the first commit in this PR):
image

Rendered (proposed approach, this is the second commit in this PR):
image

I haven't updated the code for the new approach, but if this sounds more reasonable, I'll update the drivers.

@lidavidm
Copy link
Member Author

I suppose, @pitrou @kou @hannes if you have thoughts about how to achieve ABI compatibility/on the two approaches above - AFAIK this is the last "spec" thing before I turn my focus fully towards implementation (except for @zeroshade adding some option definitions for transaction isolation levels in #97)

@kou
Copy link
Member

kou commented Sep 1, 2022

+1

@pitrou
Copy link
Member

pitrou commented Sep 1, 2022

I do not know what kind of changes may be expected in the future so I have no idea which approach would be better.

Wording-wise, "add and remove new functions" sounds weird - how do you remove new functions?

@lidavidm
Copy link
Member Author

lidavidm commented Sep 1, 2022

Wording-wise, "add and remove new functions" sounds weird - how do you remove new functions?

Whoops, that should be "add new functions and remove existing ones" - will adjust

@lidavidm lidavidm marked this pull request as ready for review September 1, 2022 16:21
@lidavidm
Copy link
Member Author

lidavidm commented Sep 6, 2022

Assuming no more comments, I'll update this to reflect the versioning strategy Kou proposed, and then merge this

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

FILL_DEFAULT(driver, StatementSetOption);
FILL_DEFAULT(driver, StatementSetSqlQuery);
FILL_DEFAULT(driver, StatementSetSubstraitPlan);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we return a non-ADBC_STATUS_OK status if version != ADBC_VERSION_1_0_0 here?

@zeroshade
Copy link
Member

zeroshade commented Sep 7, 2022

@lidavidm How frequently do we expect to be making backwards-incompatible changes that will bump the major version number? Specifically, I'm wondering if we should start with the Go package being v1 in the path, or leave it as is and wait to introduce the major version into the import path until v2. (for example, importing the main arrow Go packages is github.com/apache/arrow/go/v10/arrow and so on currently.

For everything else, this LGTM

@lidavidm
Copy link
Member Author

lidavidm commented Sep 7, 2022

I would think fairly infrequently in aggregate, but I wouldn't be surprised if a v2 comes relatively quickly once we get some real-world feedback. Same with Flight: most of what was there originally didn't change, but several things got patched or added quickly, and even still there's 1 or 2 more things we probably want to add (though not things that are critical to do right now)

@zeroshade
Copy link
Member

Okay, then I'll leave the v1 out of the import path for now until we smooth out a full release process and such and we can add the v2 to the import path along with whatever set-up we'll do for a reasonable release process (such as scripting the major version bump if necessary etc.)

btw: I don't know if we want to have this before we do the v1 release or not, but I'll be putting up a PR tomorrow which will include a Go wrapper for the C adbc driver manager via CGO.

@lidavidm
Copy link
Member Author

lidavidm commented Sep 7, 2022

Based on the versioning scheme we can have a 1.0.0 API spec without the 1.0.0 libraries quite yet ready so I think there's no rush

@lidavidm lidavidm merged commit b7475a0 into apache:main Sep 7, 2022
@lidavidm lidavidm deleted the lidavidm/compat branch September 7, 2022 15:51
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.

[Format] Document compatibility goals
4 participants