You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2024/08/21 20:45:19 UTC
[PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
zeroshade opened a new pull request, #43781:
URL: https://github.com/apache/arrow/pull/43781
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). 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](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Allowing creation of the prepared statement object outside of the client allows for logging, proxying, and handing off prepared statements if necessary.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
### Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43781:
URL: https://github.com/apache/arrow/pull/43781#issuecomment-2306109162
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 76e0f6254b75509d83e44fe8997bd14007907c4f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/29146539296) has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #43781:
URL: https://github.com/apache/arrow/pull/43781#issuecomment-2305463970
@rshura Since we provide `NewPreparedStatement` which takes a client and handle, could you just easily construct the interface that you're requesting in a generic way?
```go
type HandleGetter interface {
GetPreparedStatementHandle() []byte
}
func NewPreparedStatementFromCmd(client *Client, handle HandleGetter) *PreparedStatement {
return flightsql.NewPreparedStatement(client, handle.GetPreparedStatementHandle())
}
```
I started adding what you requested, but it ended up feeling redundant to be in the flightsql lib directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #43781:
URL: https://github.com/apache/arrow/pull/43781
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43781:
URL: https://github.com/apache/arrow/pull/43781#issuecomment-2302983362
:warning: GitHub issue #43764 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on PR #43781:
URL: https://github.com/apache/arrow/pull/43781#issuecomment-2305468386
@zeroshade Yes that's what I'm doing now. No worries and thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43764: [Go][FlightSQL] Add NewPreparedStatement function [arrow]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on PR #43781:
URL: https://github.com/apache/arrow/pull/43781#issuecomment-2304767614
This looks great, thanks!
Another minor wish-list item: can there also be an interface like this:
```
// HandleGetter is anything implementing GetPreparedStatementHandle
type HandleGetter interface {
GetPreparedStatementHandle() []byte
}
```
and then a `PreparedStatement` constructor that takes a client and a `HandleGetter`? e.g.
```
func NewPreparedStatementFromCmd(client *Client, handleGetter HandleGetter) *PreparedStatement {
...
}
```
so that I can call it with request/command objects?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org