You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vibhatha (via GitHub)" <gi...@apache.org> on 2023/05/13 11:48:56 UTC
[GitHub] [arrow] vibhatha opened a new pull request, #35580: feat: table.String()
vibhatha opened a new pull request, #35580:
URL: https://github.com/apache/arrow/pull/35580
This PR fixes https://github.com/apache/arrow/issues/35296
--
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] vibhatha commented on pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1696782469
@zeroshade one CI is failing not exactly sure if it is related.
--
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] amol- commented on pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "amol- (via GitHub)" <gi...@apache.org>.
amol- commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1695506422
@vibhatha Could you please address any pending review? So that @zeroshade can re-review
--
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 #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35580:
URL: https://github.com/apache/arrow/pull/35580#discussion_r1196648701
##########
go/arrow/array/table.go:
##########
@@ -257,6 +258,32 @@ func (tbl *simpleTable) Release() {
}
}
+func (tbl *simpleTable) String() string {
+ o := new(strings.Builder)
+ o.WriteString(tbl.Schema().String())
+ o.WriteString("\n")
+
+ for i := 0; i < int(tbl.NumCols()); i++ {
+ col := tbl.Column(i)
+ chunked := col.Data()
+ chunks := chunked.Chunks()
+ o_array := new(strings.Builder)
+ o_array.WriteString(col.Field().Name + ": [")
Review Comment:
why the need for the separate builder? why not just keep writing to `o` the whole time?
--
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 #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #35580:
URL: https://github.com/apache/arrow/pull/35580
--
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] vibhatha commented on pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1549974778
@zeroshade addressed the reviews.
--
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 #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35580:
URL: https://github.com/apache/arrow/pull/35580#discussion_r1196651687
##########
go/arrow/array/table.go:
##########
@@ -257,6 +258,32 @@ func (tbl *simpleTable) Release() {
}
}
+func (tbl *simpleTable) String() string {
+ o := new(strings.Builder)
+ o.WriteString(tbl.Schema().String())
+ o.WriteString("\n")
+
+ for i := 0; i < int(tbl.NumCols()); i++ {
+ col := tbl.Column(i)
+ chunked := col.Data()
+ chunks := chunked.Chunks()
+ o_array := new(strings.Builder)
+ o_array.WriteString(col.Field().Name + ": [")
+ numChunks := len(chunks)
+ for j := 0; j < numChunks; j++ {
+ chunk := chunked.Chunk(j)
+ if j != numChunks-1 {
+ o_array.WriteString(chunk.String() + ", ")
+ } else {
+ o_array.WriteString(chunk.String())
+ }
+
Review Comment:
This should be rewritten as
```go
for j, chunk := range col.Data().Chunks() {
if j != 0 {
o.WriteString(", ")
}
o.WriteString(chunk.String())
}
```
--
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 #35580: feat: table.String()
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1546631339
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
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}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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] vibhatha commented on a diff in pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #35580:
URL: https://github.com/apache/arrow/pull/35580#discussion_r1196680118
##########
go/arrow/array/table.go:
##########
@@ -257,6 +258,32 @@ func (tbl *simpleTable) Release() {
}
}
+func (tbl *simpleTable) String() string {
+ o := new(strings.Builder)
+ o.WriteString(tbl.Schema().String())
+ o.WriteString("\n")
+
+ for i := 0; i < int(tbl.NumCols()); i++ {
+ col := tbl.Column(i)
+ chunked := col.Data()
+ chunks := chunked.Chunks()
+ o_array := new(strings.Builder)
+ o_array.WriteString(col.Field().Name + ": [")
Review Comment:
yeah we could use the `o`. Added just for the readability.
--
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 #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #35580:
URL: https://github.com/apache/arrow/pull/35580#discussion_r1196704225
##########
go/arrow/array/table.go:
##########
@@ -257,6 +258,32 @@ func (tbl *simpleTable) Release() {
}
}
+func (tbl *simpleTable) String() string {
+ o := new(strings.Builder)
+ o.WriteString(tbl.Schema().String())
+ o.WriteString("\n")
+
+ for i := 0; i < int(tbl.NumCols()); i++ {
+ col := tbl.Column(i)
+ chunked := col.Data()
+ chunks := chunked.Chunks()
+ o_array := new(strings.Builder)
+ o_array.WriteString(col.Field().Name + ": [")
Review Comment:
I'd prefer to avoid the extra builder because of the cost of creating a new builder for every column. It would be more readable to avoid the intermediate string builder (not to mention potentially large allocations as it ends up having to deep copy the string)
--
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] vibhatha commented on a diff in pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #35580:
URL: https://github.com/apache/arrow/pull/35580#discussion_r1308077557
##########
go/arrow/array/table.go:
##########
@@ -257,6 +258,32 @@ func (tbl *simpleTable) Release() {
}
}
+func (tbl *simpleTable) String() string {
+ o := new(strings.Builder)
+ o.WriteString(tbl.Schema().String())
+ o.WriteString("\n")
+
+ for i := 0; i < int(tbl.NumCols()); i++ {
+ col := tbl.Column(i)
+ chunked := col.Data()
+ chunks := chunked.Chunks()
+ o_array := new(strings.Builder)
+ o_array.WriteString(col.Field().Name + ": [")
Review Comment:
You're correct, I updated the code.
--
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] vibhatha commented on pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1696609173
Updated the code and waiting for CIs.
cc @amol- I will ping for reviews once this stage is in clear.
--
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 #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1546631896
* Closes: #35296
--
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 #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1548816959
@vibhatha please add a unit test for 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] vibhatha commented on pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1695508534
@amol- sure I will address them soon. Sorry about the delay.
--
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] vibhatha commented on a diff in pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "vibhatha (via GitHub)" <gi...@apache.org>.
vibhatha commented on code in PR #35580:
URL: https://github.com/apache/arrow/pull/35580#discussion_r1308077824
##########
go/arrow/array/table.go:
##########
@@ -257,6 +258,32 @@ func (tbl *simpleTable) Release() {
}
}
+func (tbl *simpleTable) String() string {
+ o := new(strings.Builder)
+ o.WriteString(tbl.Schema().String())
+ o.WriteString("\n")
+
+ for i := 0; i < int(tbl.NumCols()); i++ {
+ col := tbl.Column(i)
+ chunked := col.Data()
+ chunks := chunked.Chunks()
+ o_array := new(strings.Builder)
+ o_array.WriteString(col.Field().Name + ": [")
+ numChunks := len(chunks)
+ for j := 0; j < numChunks; j++ {
+ chunk := chunked.Chunk(j)
+ if j != numChunks-1 {
+ o_array.WriteString(chunk.String() + ", ")
+ } else {
+ o_array.WriteString(chunk.String())
+ }
+
Review Comment:
I updated the code. :+1:
--
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] conbench-apache-arrow[bot] commented on pull request #35580: GH-35296: [Go] Add arrow.Table.String()
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #35580:
URL: https://github.com/apache/arrow/pull/35580#issuecomment-1700395154
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit f36855a3110a09d408df58582ee2c6341a0cdb48.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/16370187996) has more details. It also includes information about 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