-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-37720: [Go][FlightSQL] Add prepared statement handle to DoPut result #40311
GH-37720: [Go][FlightSQL] Add prepared statement handle to DoPut result #40311
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
I mentioned here in the Rust PR that I would like to expand the Go tests to cover both the stateful and stateless cases, but I didn't spend much time learning how the Go test suite is structured, and I am less familiar with Go as a language in general. I will take a look at it this week, but would appreciate recommendations of where to look from anyone. |
76533a9
to
566cdbc
Compare
@erratic-pattern thanks for doing this. As far as how the tests are structured, there's three primary files specific to flightsql:
Hope that helps! If you want me to look closely at this while it's still a draft let me know, otherwise I'll wait until you mark it as |
@zeroshade A review would be much appreciated. The PR is in draft state until the spec is voted on, but the code change is more or less "Done" |
@erratic-pattern in general this looks okay, but the build needs to be fixed as I'm seeing
In the CI |
8bf17ba
to
70e7ae5
Compare
I must have forgotten to commit the protobuf generated code in this branch. I've run |
@zeroshade Not sure how to interpret this CI failure. It seems to involve a Java client integration test. Can you take a look? https://github.com/apache/arrow/actions/runs/8352550401/job/22934920078?pr=40311 |
The proposal for this specification change was approved on the Arrow dev mailing list. See thread here I've moved this PR out of draft status. |
…ments (#40243) documents changes for stateless management of FlightSQL prepared statement handles based on the design proposal described in #37720 * GitHub Issue: #37720 PRs for language implementations: * Rust: apache/arrow-rs#5433 * Go: #40311 Mailing list discussion: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq --------- Co-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@lidavidm @danepitkin Would either of you be able to look into the Java side here and confirm if the issue is on the Java client or if the Go side is doing something wrong? The Java flight client appears to be reporting a memory leak in two scenarios. |
Shouldn't there be updates to client.go here? |
Probably, since we'd want to expose the new functionality for that. But that wouldn't explain the java failure though |
Right I was commenting before the page refreshed |
Most likely, you should split this into its own set of tests because it looks like the test is being changed in an incompatible way. |
oh wait hmm, we're not changing the handle yet? |
Looks like I forgot to add this. I'll take a look. |
I see that |
ea854e4
to
03c6461
Compare
03c6461
to
64088a2
Compare
0b2730c
to
c4aab03
Compare
Okay, tests have been updated and are passing locally. The new test failure in CI is a bit of a mystery.
I can't see why this would be related to the changes here. |
@erratic-pattern that failure isn't related to this change, I've seen it a few times but haven't yet been able to figure out the cause. It appears to be a race condition of some sort but I'm unsure of the cause. |
@zeroshade just to confirm, I assume were blocked on merging this until that unrelated change is resolved? |
@matthewmturner We don't need to wait for that unrelated issue to be addressed. Once the merge conflict is addressed, i'll merge this |
@zeroshade awesome thanks! @erratic-pattern are you able to take care of that conflict or would you like some help? |
@zeroshade @erratic-pattern i can give this another couple days but after that might make a separate PR with these changes copied and then ill see if i can resolve conflicts (not familiar with this repo though and havent looked at the conflicts yet so unsure). the reason im pressing is that having this feature is a blocker for us and we would like to avoid having to fork. |
…ateless-prepared-statement-params-go
Done. I resolved the conflict by re-running |
@erratic-pattern awesome thanks! |
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 2cf844d. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
This doesn't seem to actually work @erratic-pattern @zeroshade We capture the new value, but we continue to use the old |
…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…t result (apache#40311) ### Rationale for this change See discussion on apache#37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq ### What changes are included in this PR? Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the `DoPut(PreparedStatement)` RPC call. ### Are these changes tested? ### Are there any user-facing changes? See parent issue and docs PR apache#40243 for details of user facing changes. **This PR includes breaking changes to public APIs.** * GitHub Issue: apache#37720 Lead-authored-by: Adam Curtis <adam.curtis.dev@gmail.com> Co-authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
See discussion on #37720 and mailing list: https://lists.apache.org/thread/3kb82ypx99q96g84qv555l6x8r0bppyq
What changes are included in this PR?
Changes the Go FlightSQL client and server implementations to support returning an updated prepared statement handle to the client as part of the
DoPut(PreparedStatement)
RPC call.Are these changes tested?
Are there any user-facing changes?
See parent issue and docs PR #40243 for details of user facing changes.
This PR includes breaking changes to public APIs.