You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/19 20:50:43 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
lidavidm opened a new pull request, #33795:
URL: https://github.com/apache/arrow/pull/33795
### Are there any user-facing changes?
Adds a new method to PreparedStatement.
--
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
[GitHub] [arrow] ursabot commented on pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #33795:
URL: https://github.com/apache/arrow/pull/33795#issuecomment-1399012764
Benchmark runs are scheduled for baseline = 0e4a2e19e36d70a3072ce5275129d15fdb187c64 and contender = f9ce32ebab5071b8fc48a135a730c22313aaf9b3. f9ce32ebab5071b8fc48a135a730c22313aaf9b3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/deff7301a8094167a5f3458685d0c0e7...4688a72856d74b63b4494800e30e4bfe/)
[Failed] [test-mac-arm](https://conbench.ursa.dev/compare/runs/4ff6080bb6794a91b251f13e894603e4...387f736caaf5494d9cad1f4081ca78d4/)
[Failed] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/48362c3cb97c43f2afb7c20547cf4ad0...aff01843b1ca4ea79a6a5cbdeddc1644/)
[Failed] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/747a35465dfa415fa603317ae0ca67e0...3cad7f913b124ec1af1cd10ff0385c78/)
Buildkite builds:
[Failed] [`f9ce32eb` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2237)
[Failed] [`f9ce32eb` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2262)
[Failed] [`f9ce32eb` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2232)
[Failed] [`f9ce32eb` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2255)
[Failed] [`0e4a2e19` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2236)
[Failed] [`0e4a2e19` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2261)
[Failed] [`0e4a2e19` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2231)
[Failed] [`0e4a2e19` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2254)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] lidavidm commented on a diff in pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #33795:
URL: https://github.com/apache/arrow/pull/33795#discussion_r1082956468
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -491,6 +490,33 @@ func (p *PreparedStatement) ExecuteUpdate(ctx context.Context) (nrecords int64,
return updateResult.GetRecordCount(), nil
}
+func (p *PreparedStatement) hasBindParameters() bool {
+ return (p.paramBinding != nil && p.paramBinding.NumRows() > 0) || (p.streamBinding != nil)
+}
+
+func (p *PreparedStatement) writeBindParameters(pstream pb.FlightService_DoPutClient, desc *pb.FlightDescriptor) (*flight.Writer, error) {
+ if p.paramBinding != nil {
+ wr := flight.NewRecordWriter(pstream, ipc.WithSchema(p.paramBinding.Schema()))
+ wr.SetFlightDescriptor(desc)
+ if err := wr.Write(p.paramBinding); err != nil {
+ return nil, err
+ }
+ return wr, nil
+ } else {
+ wr := flight.NewRecordWriter(pstream, ipc.WithSchema(p.streamBinding.Schema()))
+ wr.SetFlightDescriptor(desc)
+ for p.streamBinding.Next() {
+ if err := wr.Write(p.streamBinding.Record()); err != nil {
+ return nil, err
+ }
+ }
Review Comment:
Unfortunately that takes a Reader, but streamBinding is a RecordReader
--
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
[GitHub] [arrow] zeroshade merged pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #33795:
URL: https://github.com/apache/arrow/pull/33795
--
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
[GitHub] [arrow] zeroshade commented on a diff in pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by GitBox <gi...@apache.org>.
zeroshade commented on code in PR #33795:
URL: https://github.com/apache/arrow/pull/33795#discussion_r1082759927
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -518,22 +544,44 @@ func (p *PreparedStatement) GetSchema(ctx context.Context) (*flight.SchemaResult
return p.client.getSchema(ctx, desc, p.opts...)
}
-// SetParameters takes a record batch to send as the parameter bindings when
-// executing. It should match the schema from ParameterSchema.
-//
-// This will call Retain on the record to ensure it doesn't get released
-// out from under the statement. Release will be called on a previous
-// binding record if it existed, and will be called upon calling Close
-// on the PreparedStatement.
-func (p *PreparedStatement) SetParameters(binding arrow.Record) {
+func (p *PreparedStatement) clearParameters() {
if p.paramBinding != nil {
p.paramBinding.Release()
p.paramBinding = nil
}
+ if p.streamBinding != nil {
+ p.streamBinding.Release()
+ p.streamBinding = nil
+ }
+}
+
+// SetParameters takes a record batch to send as the parameter bindings when
+// executing. It should match the schema from ParameterSchema.
+//
+// This will call Retain on the record to ensure it doesn't get released out
+// from under the statement. Release will be called on a previous binding
+// record or reader if it existed, and will be called upon calling Close on the
+// PreparedStatement.
+func (p *PreparedStatement) SetParameters(binding arrow.Record) {
+ p.clearParameters()
p.paramBinding = binding
p.paramBinding.Retain()
}
Review Comment:
should probably mention in the docstring that it will clear any existing bound params
--
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
[GitHub] [arrow] zeroshade commented on a diff in pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by GitBox <gi...@apache.org>.
zeroshade commented on code in PR #33795:
URL: https://github.com/apache/arrow/pull/33795#discussion_r1082761204
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -518,22 +544,44 @@ func (p *PreparedStatement) GetSchema(ctx context.Context) (*flight.SchemaResult
return p.client.getSchema(ctx, desc, p.opts...)
}
-// SetParameters takes a record batch to send as the parameter bindings when
-// executing. It should match the schema from ParameterSchema.
-//
-// This will call Retain on the record to ensure it doesn't get released
-// out from under the statement. Release will be called on a previous
-// binding record if it existed, and will be called upon calling Close
-// on the PreparedStatement.
-func (p *PreparedStatement) SetParameters(binding arrow.Record) {
+func (p *PreparedStatement) clearParameters() {
if p.paramBinding != nil {
p.paramBinding.Release()
p.paramBinding = nil
}
+ if p.streamBinding != nil {
+ p.streamBinding.Release()
+ p.streamBinding = nil
+ }
+}
+
+// SetParameters takes a record batch to send as the parameter bindings when
+// executing. It should match the schema from ParameterSchema.
+//
+// This will call Retain on the record to ensure it doesn't get released out
+// from under the statement. Release will be called on a previous binding
+// record or reader if it existed, and will be called upon calling Close on the
+// PreparedStatement.
+func (p *PreparedStatement) SetParameters(binding arrow.Record) {
+ p.clearParameters()
p.paramBinding = binding
p.paramBinding.Retain()
}
Review Comment:
nvm just saw you have `Release will be called on a previous binding record or reader`, ignore this :)
--
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
[GitHub] [arrow] zeroshade commented on a diff in pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by GitBox <gi...@apache.org>.
zeroshade commented on code in PR #33795:
URL: https://github.com/apache/arrow/pull/33795#discussion_r1082757863
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -491,6 +490,33 @@ func (p *PreparedStatement) ExecuteUpdate(ctx context.Context) (nrecords int64,
return updateResult.GetRecordCount(), nil
}
+func (p *PreparedStatement) hasBindParameters() bool {
+ return (p.paramBinding != nil && p.paramBinding.NumRows() > 0) || (p.streamBinding != nil)
+}
+
+func (p *PreparedStatement) writeBindParameters(pstream pb.FlightService_DoPutClient, desc *pb.FlightDescriptor) (*flight.Writer, error) {
+ if p.paramBinding != nil {
+ wr := flight.NewRecordWriter(pstream, ipc.WithSchema(p.paramBinding.Schema()))
+ wr.SetFlightDescriptor(desc)
+ if err := wr.Write(p.paramBinding); err != nil {
+ return nil, err
+ }
+ return wr, nil
+ } else {
+ wr := flight.NewRecordWriter(pstream, ipc.WithSchema(p.streamBinding.Schema()))
+ wr.SetFlightDescriptor(desc)
+ for p.streamBinding.Next() {
+ if err := wr.Write(p.streamBinding.Record()); err != nil {
+ return nil, err
+ }
+ }
Review Comment:
`arrio.Copy` ?
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #33795: GH-33794: [Go] Add SetRecordReader to PreparedStatement
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #33795:
URL: https://github.com/apache/arrow/pull/33795#issuecomment-1397590777
* Closes: #33794
--
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