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 2020/04/24 20:05:06 UTC
[GitHub] [arrow] markhildreth opened a new pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
markhildreth opened a new pull request #7035:
URL: https://github.com/apache/arrow/pull/7035
Fixes [ARROW-8590](https://issues.apache.org/jira/browse/ARROW-8590)
This builds on #6972, and thus should be merged after that PR is merged.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#issuecomment-619219686
https://issues.apache.org/jira/browse/ARROW-8590
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markhildreth commented on a change in pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
markhildreth commented on a change in pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#discussion_r417684865
##########
File path: rust/arrow/src/util/pretty.rs
##########
@@ -27,18 +27,18 @@ use prettytable::{Cell, Row, Table};
use crate::error::{ArrowError, Result};
///! Create a visual representation of record batches
-pub fn pretty_format_batches(results: &Vec<RecordBatch>) -> Result<String> {
Review comment:
Something to point out: one thing I noticed after going back was realizing that this API could be changed from a Vec to the less specific slice without any other changes necessary. Ideally, this should have been done on the last PR (#6972) but I didn't want to hold that PR up any longer. If there are any qualms I can undo 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#issuecomment-621511978
I just merged https://github.com/apache/arrow/pull/6972 ... could you rebase when you get a chance?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#discussion_r418346377
##########
File path: rust/arrow/src/util/pretty.rs
##########
@@ -27,18 +27,18 @@ use prettytable::{Cell, Row, Table};
use crate::error::{ArrowError, Result};
///! Create a visual representation of record batches
-pub fn pretty_format_batches(results: &Vec<RecordBatch>) -> Result<String> {
Review comment:
I agree. I got into a bad habit of using &Vec instead of slice early on and have been meaning to start fixing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markhildreth commented on pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
markhildreth commented on pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#issuecomment-621544410
@andygrove All set.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markhildreth commented on a change in pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
markhildreth commented on a change in pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#discussion_r417684865
##########
File path: rust/arrow/src/util/pretty.rs
##########
@@ -27,18 +27,18 @@ use prettytable::{Cell, Row, Table};
use crate::error::{ArrowError, Result};
///! Create a visual representation of record batches
-pub fn pretty_format_batches(results: &Vec<RecordBatch>) -> Result<String> {
Review comment:
Something to point out: one thing I noticed that thought was a good change was to make this API. Technically should have been done on the last PR (#6972) but I didn't want to hold that PR up any longer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markhildreth commented on a change in pull request #7035: ARROW-8590: [Rust] Use arrow crate pretty util in DataFusion
Posted by GitBox <gi...@apache.org>.
markhildreth commented on a change in pull request #7035:
URL: https://github.com/apache/arrow/pull/7035#discussion_r417684865
##########
File path: rust/arrow/src/util/pretty.rs
##########
@@ -27,18 +27,18 @@ use prettytable::{Cell, Row, Table};
use crate::error::{ArrowError, Result};
///! Create a visual representation of record batches
-pub fn pretty_format_batches(results: &Vec<RecordBatch>) -> Result<String> {
Review comment:
Something to point out: one thing I noticed after going back was realizing that this API could be changed from a Vec to the less specific slice without any change necessary. Ideally, this should have been done on the last PR (#6972) but I didn't want to hold that PR up any longer. If there are any qualms I can undo 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org