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 2021/11/16 16:48:52 UTC
[GitHub] [arrow] chriscasola opened a new pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
chriscasola opened a new pull request #11712:
URL: https://github.com/apache/arrow/pull/11712
Allocate the body of messages from the memory.Allocator instead
of using make.
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r750484716
##########
File path: go/arrow/ipc/message.go
##########
@@ -154,11 +154,18 @@ type messageReader struct {
refCount int64
msg *Message
+
+ mem memory.Allocator
}
// NewMessageReader returns a reader that reads messages from an input stream.
-func NewMessageReader(r io.Reader) MessageReader {
- return &messageReader{r: r, refCount: 1}
+func NewMessageReader(r io.Reader, opts ...Option) MessageReader {
Review comment:
You're right about avoiding breaking with the options. Though I personally doubt many people are calling `NewMessageReader` directly, it's really only exported because the `flight` package uses it to sit on top of the ipc readers to provide access to the additional metadata in flight messages.
It's not a big issue either way I think. More of an aesthetic thing where this gives the impression that message reader would work with other options that get ignored like the reader is doing now.
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751414434
##########
File path: go/arrow/ipc/message.go
##########
@@ -224,12 +231,14 @@ func (r *messageReader) Message() (*Message, error) {
meta := flatbuf.GetRootAsMessage(buf, 0)
bodyLen := meta.BodyLength()
- buf = make([]byte, bodyLen)
- _, err = io.ReadFull(r.r, buf)
+ body := memory.NewResizableBuffer(r.mem)
+ defer body.Release()
Review comment:
the message in `newMessageFromFB` calls retain on the body buffer?
--
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] chriscasola commented on a change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751416920
##########
File path: go/arrow/memory/checked_allocator.go
##########
@@ -21,12 +21,13 @@ import (
"runtime"
"strconv"
"sync"
+ "sync/atomic"
"unsafe"
)
type CheckedAllocator struct {
mem Allocator
- sz int
+ sz int64
Review comment:
There is no `atomic.LoadInt`
--
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] chriscasola commented on pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-973035642
@zeroshade this is ready for another look.
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751413342
##########
File path: go/arrow/ipc/message.go
##########
@@ -154,11 +154,18 @@ type messageReader struct {
refCount int64
msg *Message
+
+ mem memory.Allocator
}
// NewMessageReader returns a reader that reads messages from an input stream.
-func NewMessageReader(r io.Reader) MessageReader {
- return &messageReader{r: r, refCount: 1}
+func NewMessageReader(r io.Reader, opts ...Option) MessageReader {
Review comment:
I agree with you on the `WithSchema` comment as far as for the writer (it's not required for the reader which will read the schema from the stream, and you only use `WithSchema` for asserting that you're reading a specific schema).
For now we'll leave it, but after the release of v6.0.1 which will properly tag this so that people can upgrade to v6, and people start using the tagged releases, then i'll be comfortable making the breaking change since it would be in a tagged *major release* that would make sense to allow a breaking change in.
--
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] chriscasola commented on a change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751419206
##########
File path: go/arrow/ipc/message.go
##########
@@ -224,12 +231,14 @@ func (r *messageReader) Message() (*Message, error) {
meta := flatbuf.GetRootAsMessage(buf, 0)
bodyLen := meta.BodyLength()
- buf = make([]byte, bodyLen)
- _, err = io.ReadFull(r.r, buf)
+ body := memory.NewResizableBuffer(r.mem)
+ defer body.Release()
Review comment:
`newMessageFromFB` calls `Retain` on the body.
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751409009
##########
File path: go/arrow/ipc/file_reader.go
##########
@@ -337,6 +341,11 @@ func newRecord(schema *arrow.Schema, meta *memory.Buffer, body ReadAtSeeker) arr
for i, field := range schema.Fields() {
cols[i] = ctx.loadArray(field.Type)
}
+ defer func() {
+ for _, c := range cols {
+ c.Release()
+ }
+ }()
Review comment:
```go
for i, field := range schema.Fields() {
cols[i] = ctx.loadArray(field.Type)
defer cols[i].Release()
}
```
No need for the separate defer func and loop
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r750475818
##########
File path: go/arrow/ipc/message.go
##########
@@ -154,11 +154,18 @@ type messageReader struct {
refCount int64
msg *Message
+
+ mem memory.Allocator
}
// NewMessageReader returns a reader that reads messages from an input stream.
-func NewMessageReader(r io.Reader) MessageReader {
- return &messageReader{r: r, refCount: 1}
+func NewMessageReader(r io.Reader, opts ...Option) MessageReader {
Review comment:
do we really need to have a full options addition here as opposed to just taking the allocator as a param? Not sure if there's any other configurable options we'd want to be adding in the future, particularly if we're going to ignore any options that aren't the allocator here.
--
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 #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-973222266
Benchmark runs are scheduled for baseline = d211b4448f5e139fec1e4c563eb968bf2492387e and contender = de5aa544275c9664d17874b1b93de195fb619825. de5aa544275c9664d17874b1b93de195fb619825 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e9166984c0ad481c8fb0b55cb10b1c91...0bcdd7a1839f4b0ab30b9ca51119e57b/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f344674b0bfc41fca6581cffc1431545...39bca753e9e0466ab0f897b53469a3a0/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/da8285c0a7164da4baa03ef65513bbf5...ce5e6f5ec2534508bdefda7866cdb15c/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751416375
##########
File path: go/arrow/memory/checked_allocator.go
##########
@@ -21,12 +21,13 @@ import (
"runtime"
"strconv"
"sync"
+ "sync/atomic"
"unsafe"
)
type CheckedAllocator struct {
mem Allocator
- sz int
+ sz int64
Review comment:
why this change? it only really affects when this is built and run on a 32-bit machine since an `int` will be 64bits on 64-bit architectures
--
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 edited a comment on pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-973222266
Benchmark runs are scheduled for baseline = d211b4448f5e139fec1e4c563eb968bf2492387e and contender = de5aa544275c9664d17874b1b93de195fb619825. de5aa544275c9664d17874b1b93de195fb619825 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e9166984c0ad481c8fb0b55cb10b1c91...0bcdd7a1839f4b0ab30b9ca51119e57b/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f344674b0bfc41fca6581cffc1431545...39bca753e9e0466ab0f897b53469a3a0/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/da8285c0a7164da4baa03ef65513bbf5...ce5e6f5ec2534508bdefda7866cdb15c/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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] chriscasola commented on a change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r750476945
##########
File path: go/arrow/ipc/message.go
##########
@@ -154,11 +154,18 @@ type messageReader struct {
refCount int64
msg *Message
+
+ mem memory.Allocator
}
// NewMessageReader returns a reader that reads messages from an input stream.
-func NewMessageReader(r io.Reader) MessageReader {
- return &messageReader{r: r, refCount: 1}
+func NewMessageReader(r io.Reader, opts ...Option) MessageReader {
Review comment:
I wanted to avoid a breaking change by adding a param. Is that not an issue? Or are you suggesting a new function?
--
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] chriscasola commented on pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-971595923
@zeroshade should be good now.
--
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 pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-970617219
@chriscasola make sure to properly call `.Release` on the new `memory.Buffer`s you're creating since now you're building them off the allocator, and then I'd say it's a good idea to push it down to the decompression logic here too, otherwise it still won't be accurate accounting i think.
--
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] chriscasola commented on a change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751417657
##########
File path: go/arrow/ipc/file_reader.go
##########
@@ -337,6 +341,11 @@ func newRecord(schema *arrow.Schema, meta *memory.Buffer, body ReadAtSeeker) arr
for i, field := range schema.Fields() {
cols[i] = ctx.loadArray(field.Type)
}
+ defer func() {
+ for _, c := range cols {
+ c.Release()
+ }
+ }()
Review comment:
I didn't want to call `defer` inside a loop - is that ok?
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751679489
##########
File path: go/arrow/ipc/file_reader.go
##########
@@ -337,6 +341,11 @@ func newRecord(schema *arrow.Schema, meta *memory.Buffer, body ReadAtSeeker) arr
for i, field := range schema.Fields() {
cols[i] = ctx.loadArray(field.Type)
}
+ defer func() {
+ for _, c := range cols {
+ c.Release()
+ }
+ }()
Review comment:
yup, it's perfectly okay. Remember that defer's only run at the end of the enclosing *function* not scope.
--
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 pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-970479777
Honestly, looking at this and now that I think of it. It looks like the readers actually call `bytes.NewReader` on the buffer that was already created in the message reader. As a result, it looks like we're currently actually copying no matter what which sucks, but makes it easy to stick the allocator in there.
Optimally, we'd do what you've done here with the allocator in the message reader for the body, and then just pass slices of this buffer when creating the records. Though that would probably be beyond the scope of this change.
--
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 #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-970462578
--
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 pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-973070463
@chriscasola This LGTM, once all the checks pass I'll merge it.
--
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] asfgit closed pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #11712:
URL: https://github.com/apache/arrow/pull/11712
--
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 edited a comment on pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-973222266
Benchmark runs are scheduled for baseline = d211b4448f5e139fec1e4c563eb968bf2492387e and contender = de5aa544275c9664d17874b1b93de195fb619825. de5aa544275c9664d17874b1b93de195fb619825 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e9166984c0ad481c8fb0b55cb10b1c91...0bcdd7a1839f4b0ab30b9ca51119e57b/)
[Finished :arrow_down:9.74% :arrow_up:2.56%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f344674b0bfc41fca6581cffc1431545...39bca753e9e0466ab0f897b53469a3a0/)
[Finished :arrow_down:0.13% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/da8285c0a7164da4baa03ef65513bbf5...ce5e6f5ec2534508bdefda7866cdb15c/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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 pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-970476332
You are also going to want to modify https://github.com/apache/arrow/blob/master/go/arrow/ipc/file_reader.go#L350 where the decompression is handled.
--
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] chriscasola commented on a change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r750566701
##########
File path: go/arrow/ipc/message.go
##########
@@ -154,11 +154,18 @@ type messageReader struct {
refCount int64
msg *Message
+
+ mem memory.Allocator
}
// NewMessageReader returns a reader that reads messages from an input stream.
-func NewMessageReader(r io.Reader) MessageReader {
- return &messageReader{r: r, refCount: 1}
+func NewMessageReader(r io.Reader, opts ...Option) MessageReader {
Review comment:
The use of the options pattern is a bit confusing in this library, especially the `WithSchema` option, since in a lot of cases it's not optional and the function will panic without it. In my opinion required things should be real parameters.
I'm going to leave this as-is unless you feel strongly about making it a normal parameter and are okay with the breaking change.
--
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] chriscasola commented on pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
chriscasola commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-970589151
@zeroshade I see the issue with `bytes.NewReader` but agree that sounds like a more involved change. Should I still modify the decompression logic in this PR?
--
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 change in pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#discussion_r751415437
##########
File path: go/arrow/ipc/reader.go
##########
@@ -75,7 +75,7 @@ func NewReaderFromMessageReader(r MessageReader, opts ...Option) (*Reader, error
// NewReader returns a reader that reads records from an input stream.
func NewReader(r io.Reader, opts ...Option) (*Reader, error) {
- return NewReaderFromMessageReader(NewMessageReader(r), opts...)
+ return NewReaderFromMessageReader(NewMessageReader(r, opts...), opts...)
Review comment:
ugh, this feels horrible, but i can't think of a good way to improve this at the moment....
--
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 pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-971748641
@chriscasola LGTM for the most part, just a couple of comments i'd like addressed first.
--
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 edited a comment on pull request #11712: ARROW-14717: [Go] Use the ipc.Reader allocator in messageReader
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11712:
URL: https://github.com/apache/arrow/pull/11712#issuecomment-973222266
Benchmark runs are scheduled for baseline = d211b4448f5e139fec1e4c563eb968bf2492387e and contender = de5aa544275c9664d17874b1b93de195fb619825. de5aa544275c9664d17874b1b93de195fb619825 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/e9166984c0ad481c8fb0b55cb10b1c91...0bcdd7a1839f4b0ab30b9ca51119e57b/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f344674b0bfc41fca6581cffc1431545...39bca753e9e0466ab0f897b53469a3a0/)
[Finished :arrow_down:0.13% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/da8285c0a7164da4baa03ef65513bbf5...ce5e6f5ec2534508bdefda7866cdb15c/)
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True
--
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