GH-1063: Add is_update field to ActionCreatePreparedStatementResult#1064
GH-1063: Add is_update field to ActionCreatePreparedStatementResult#1064ennuite wants to merge 6 commits intoapache:mainfrom
Conversation
|
Thank you for opening a pull request! Please label the PR with one or more of:
Also, add the 'breaking-change' label if appropriate. See CONTRIBUTING.md for details. |
|
@lidavidm this PR currently contains a client implementation of the changes proposed in https://lists.apache.org/thread/88msflnwkkw8t81czs2ndqhkn1fb1pxd I can expand it with a server side implementation or we can use already use this as a starting point for the discussion, let me know what you think. |
|
This should be done on apache/arrow, not apache/arrow-java, which only has a copy of the spec We will need a client and server implementation, preferably in at least two languages |
|
That said we can use this for discussion, if you want to link it from the mailing list thread. |
|
Ah you did link this already; thanks. In that case I think we just need to demonstrate the server side, etc. |
|
Sounds good @lidavidm I updated this PR with a server side implementation and tests for Statement.execute() and PreparedStatement.execute() I will implement an analogous PR in another driver. The only driver in apache/arrow is the ODBC driver but that is still unreleased and Windows only. Is it ok if I do it in the ADBC Python Flight SQL driver, which lives at apache/arrow-adbc? I can then move on to ODBC if the community tells me to go forward with the changes. I note that these changes, while affecting the protocol, are mostly about drivers, so arrow-adbc makes more sense to me. |
|
I assume you can and would need to make changes to arrow-go, arrow-cpp, etc? We can start there. |
|
Yeah, I talked with the rest of the guys at the last meeting. I will implement the same change in ADBC Python Flight SQL, I'll try to get to it this week. Then I'll send the spec change PR in apache/arrow, the PRs in Go and Python and this one to the mailing list for a vote. |
|
What happened to the idea of revising the Execute methods to allow returning either data/row count instead of having to separate the two? |
|
@lidavidm I still think that's a good idea, we talked about it during the last community meeting. I'm going to propose this fix first as it is low-hanging fruit, and we should continue to discuss that way in the mailing list. Can you expand in the mailing list on what exactly are you proposing? Do you want to use DoExchange? A new RPC? If you find it easier and you have the availability, show up in the next community meeting and we can discuss. I've been looking into how we could do this with DoExchange and have some ideas. |
|
The community meeting is unfortunately extremely late at night for me. I am basically encouraging looking at apache/arrow#41840 In particular whether a dedicated RPC for this purpose can be declared, instead of overloading more Flight RPC methods |
|
Ok @lidavidm, thanks for the pointer to that issue. I will look into it once I finish the PRs for this change, and then I'll send an email with my findings. |
What's Changed
A new field,
optional bool is_update = 4;, was added tomessage ActionCreatePreparedStatementResult. When this field is sent by the server, its value indicates whether the proper network flow to execute the query that the driver should follow usesCommandPreparedStatementQueryorCommandPreparedStatementUpdate.For outdated servers that don't send the field, the driver maintains its current behavior of using
CommandPreparedStatementQuerywhen thedataset_schemais not empty, thus ensuring the backward compatibility of the new driver with old servers.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 #1063.