Skip to content

Conversation

@howareyouman
Copy link
Contributor

@howareyouman howareyouman commented Jul 1, 2025

Moving FlightSQLDriverInit function to a separate go file.
Resolving the request from the recent PR - link

FlightSqlDriverInit and FlightSQLDriverInit are a part of legacy init functions.
We should keep them in place and not lose because of code generation.
@howareyouman howareyouman requested a review from zeroshade as a code owner July 1, 2025 09:56
@github-actions github-actions bot added this to the ADBC Libraries 19 milestone Jul 1, 2025
@zeroshade
Copy link
Member

@howareyouman the driver.go is also generated by code generation, with the goal of avoiding losing the functions during code-gen it needs to be a separate file from driver.go/utils.c/utils.h

@howareyouman
Copy link
Contributor Author

@zeroshade I see that these 2 functions heavily rely on C layer, which lives in the pkg folder, and the dependency between folders is pkg(generated) -> driver(not generated).

I don't understand how to remove them from the generation step if we want to expose them in the shared library, and they rely on generated code? What should be done here then?

@zeroshade
Copy link
Member

The generation doesn't remove and recreate the directory, we just create the files. So you can just make a new file in the pkg/flightsql directory which contains the functions. That way they don't get overwritten during generation, but are still part of the package that they rely on.

@howareyouman
Copy link
Contributor Author

@zeroshade I've created new go file with functions, removed _tmpl generated files from the repo (I don't know why we had them in the repo?).

@howareyouman howareyouman changed the title fix(go/adbc): moving the declaration of functions to go code fix(go/adbc): changing the location of init functions to new file Jul 4, 2025
@zeroshade
Copy link
Member

The tmpl files are in the repo so that we can use go generate to regenerate the files. We need them to be in the repo for the code generation. The flightsql package isn't the only one that we generate using it. It's a general purpose template for generating a C wrapper for an ADBC driver written in go.

Please restore those files as they are necessary. My suggestion was to leave the existing files as is and just add one new file containing the function we're adding back.

@howareyouman howareyouman changed the title fix(go/adbc): changing the location of init functions to new file fix(go/adbc): changing the location of FlightSQLDriverInit function Jul 4, 2025
@howareyouman
Copy link
Contributor Author

@zeroshade migrated FlightSQLDriverInit to new file

@davidhcoe
Copy link
Contributor

@howareyouman - what does this need to close out by Wednesday (early US time)?

@howareyouman
Copy link
Contributor Author

@davidhcoe I suppose I've already done what @zeroshade was asking for. The only problem is that we have CI failed, which I couldn't restart.

@zeroshade
Copy link
Member

restarted the flaky test. This looks good to me

@zeroshade zeroshade merged commit 875c0b2 into apache:main Jul 7, 2025
86 of 92 checks passed
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.

4 participants