Skip to content

feat(go/adbc/driver/flightsql): handle is_update in prepared stmts - #4161

Open
ennuite wants to merge 6 commits into
apache:mainfrom
ennuite:gh-4074-add-is-update
Open

feat(go/adbc/driver/flightsql): handle is_update in prepared stmts#4161
ennuite wants to merge 6 commits into
apache:mainfrom
ennuite:gh-4074-add-is-update

Conversation

@ennuite

@ennuiteennuite commented Mar 31, 2026

Copy link
Copy Markdown

AI Disclaimer

This change was created with AI assistance (Augment Code). All lines were manually reviewed by a human. The output is not copyrightable subject matter.

Closes#4074

@ennuite

ennuite commented Mar 31, 2026

Copy link
Copy Markdown
Author

I'm unsure of which component to use given that I touched 3 different ones: ‎go/adbc/driver/flightsql, ‎python/adbc_driver_flightsql and ‎python/adbc_driver_manager

It's my first PR in this codebase and I'm not very familiar with Go. I would appreciate a review not only on the content but also on style. I'm available for improving this as requested.

It's not yet in a state where I feel like it's ready to merge, although it is fine as a PoC.

Things I want to improve:

  1. Add some tests for the Python driver.
  2. Change the way I'm fetching the option. Details: it is being fetched in a code path that is shared by all drivers and not just by the Flight driver, and in those cases it will always throw an exception (which is caught with no side effects). This might hinder performance, and I'm not a proponent of using exceptions for control flow. I intend to move the logic to a code path specific to the Flight SQL driver and remove the exceptions.

Question: I added a logger in Python, but I don't see logging being used anywhere else. I would like to know your opinion on log statements in this codebase.

@ennuiteennuite changed the title feat: Add new network route via cursor.execute()feat: Add CommandPreparedStatementUpdate route to cursor.execute() in the Python ADBC Flight SQL driverMar 31, 2026
@ennuite

ennuite commented Mar 31, 2026

Copy link
Copy Markdown
Author

This PR is not an individual piece of work. It requires changes in Arrow-Go and the main repo.

@lidavidmlidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't make any Python changes. It should be handled entirely in the driver.

@ennuite

ennuite commented Apr 1, 2026

Copy link
Copy Markdown
Author

Don't make any Python changes. It should be handled entirely in the driver.

@lidavidm I thought about that and it would definitely be much easier. However, do we want this in ADBC Go? My understanding is that, in ADBC Go, queries that generate result sets go via Statement.executeQuery() and queries that don't go via Statement.executeUpdate(). Do we want DML queries in Statement.executeQuery()?

@lidavidm

Copy link
Copy Markdown
Member

Yes, that should be handled properly. It'll simply return an empty resultset.

@ennuite

Copy link
Copy Markdown
Author

I see you already approved. I will still add tests tomorrow, but the feature work is finished. After I add the tests, if you agree, I think it's time to put the spec change to a vote.

@ennuiteennuite changed the title feat: Add CommandPreparedStatementUpdate route to cursor.execute() in the Python ADBC Flight SQL driverfeat(‎go/adbc/driver/flightsql): Add CommandPreparedStatementUpdate route to cursor.execute() in the Python ADBC Flight SQL driverApr 3, 2026
@lidavidmlidavidm changed the title feat(‎go/adbc/driver/flightsql): Add CommandPreparedStatementUpdate route to cursor.execute() in the Python ADBC Flight SQL driverfeat(go/adbc/driver/flightsql): handle is_update in prepared stmtsApr 3, 2026
@ennuite

Copy link
Copy Markdown
Author

@lidavidm I added tests for this new behavior. I also manually tested it end-to-end, using the Go driver and the Python driver, against backend servers with and without the change to ensure backwards compatibility.

Let me know if we can proceed with voting on the spec change.

@ennuite
ennuite marked this pull request as ready for review June 8, 2026 14:22
@ennuite
ennuite requested a review from zeroshade as a code ownerJune 8, 2026 14:22
@ennuite
ennuite marked this pull request as draft July 3, 2026 03:05
@ennuite

Copy link
Copy Markdown
Author

Converted to draft until I carry over the latest changes from apache/arrow-go#732

@lidavidm

Copy link
Copy Markdown
Member

@ennuite is this still in progress?

@ennuite
ennuiteforce-pushed the gh-4074-add-is-update branch from 5df5778 to 2dbd787CompareJuly 26, 2026 23:21
@ennuite
ennuite marked this pull request as ready for review July 27, 2026 01:08
@ennuite

Copy link
Copy Markdown
Author

@lidavidm It's ready for review now. Sorry for the delay.

@lidavidmlidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems good, but we need the arrow-go changes

@zeroshade

Copy link
Copy Markdown
Member

I added a bunch of comments on the corresponding arrow-go PR that are waiting for responses

@ennuite

Copy link
Copy Markdown
Author

@lidavidm The Arrow-Go changes have been merged, see apache/arrow-go#732
Let me know if you need anything else here. Do we need to wait for an Arrow Go release before merging this?

@lidavidm

Copy link
Copy Markdown
Member

Well, yes, we can't merge in this state.

@zeroshade

Copy link
Copy Markdown
Member

I should be doing an arrow-go release in the next week or so

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants

@ennuite@lidavidm@zeroshade