You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jiangzhx (via GitHub)" <gi...@apache.org> on 2023/03/14 02:14:39 UTC
[GitHub] [arrow-datafusion] jiangzhx opened a new pull request, #5585: fix dataframe only boolean/binary column got error on describe
jiangzhx opened a new pull request, #5585:
URL: https://github.com/apache/arrow-datafusion/pull/5585
# Which issue does this PR close?
Closes #5558
- handle input dataframe has only boolean/binary column got error
- add more test
--
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-datafusion] alamb commented on a diff in pull request #5585: fix dataframe only boolean/binary column got error on describe
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5585:
URL: https://github.com/apache/arrow-datafusion/pull/5585#discussion_r1135659333
##########
datafusion/core/src/dataframe.rs:
##########
@@ -437,19 +416,27 @@ impl DataFrame {
))];
for field in original_schema_fields {
let mut array_datas = vec![];
- for record_batch in describe_record_batch.iter() {
- // safe unwrap since aggregate record batches should have at least 1 record
- let column = record_batch.get(0).unwrap().column_by_name(field.name());
- match column {
- Some(c) => {
- if field.data_type().is_numeric() {
- array_datas.push(cast(c, &DataType::Float64)?);
- } else {
- array_datas.push(cast(c, &DataType::Utf8)?);
+ for result in describe_record_batch.iter() {
+ match result {
+ Ok(df) => {
+ let record_batch = df.clone().collect().await?;
+ if record_batch.len() == 1 {
+ if let Some(column) =
+ record_batch.get(0).unwrap().column_by_name(field.name())
+ {
+ if field.data_type().is_numeric() {
+ array_datas.push(cast(column, &DataType::Float64)?);
+ } else {
+ array_datas.push(cast(column, &DataType::Utf8)?);
+ }
+ } else {
+ array_datas
+ .push(Arc::new(StringArray::from_slice(["null"])));
+ }
}
}
- //if None mean the column cannot be min/max aggregation
- None => {
+ Err(_) => {
+ //Handling error when only boolean/binary column, and in other cases
array_datas.push(Arc::new(StringArray::from_slice(["null"])));
Review Comment:
Maybe you can match on the error message
Something like
```rust
Err(e) if e.to_string().contains("message") => {
...
}
_ => {
// Default error
}
```
--
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-datafusion] alamb merged pull request #5585: fix dataframe only boolean/binary column got error on describe
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5585:
URL: https://github.com/apache/arrow-datafusion/pull/5585
--
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-datafusion] jiangzhx commented on a diff in pull request #5585: fix dataframe only boolean/binary column got error on describe
Posted by "jiangzhx (via GitHub)" <gi...@apache.org>.
jiangzhx commented on code in PR #5585:
URL: https://github.com/apache/arrow-datafusion/pull/5585#discussion_r1136557850
##########
datafusion/core/src/dataframe.rs:
##########
@@ -437,19 +416,27 @@ impl DataFrame {
))];
for field in original_schema_fields {
let mut array_datas = vec![];
- for record_batch in describe_record_batch.iter() {
- // safe unwrap since aggregate record batches should have at least 1 record
- let column = record_batch.get(0).unwrap().column_by_name(field.name());
- match column {
- Some(c) => {
- if field.data_type().is_numeric() {
- array_datas.push(cast(c, &DataType::Float64)?);
- } else {
- array_datas.push(cast(c, &DataType::Utf8)?);
+ for result in describe_record_batch.iter() {
+ match result {
+ Ok(df) => {
+ let record_batch = df.clone().collect().await?;
+ if record_batch.len() == 1 {
Review Comment:
Thanks, I made some changes. If you have time, could you please help with another code review?
##########
datafusion/core/src/dataframe.rs:
##########
@@ -437,19 +416,27 @@ impl DataFrame {
))];
for field in original_schema_fields {
let mut array_datas = vec![];
- for record_batch in describe_record_batch.iter() {
- // safe unwrap since aggregate record batches should have at least 1 record
- let column = record_batch.get(0).unwrap().column_by_name(field.name());
- match column {
- Some(c) => {
- if field.data_type().is_numeric() {
- array_datas.push(cast(c, &DataType::Float64)?);
- } else {
- array_datas.push(cast(c, &DataType::Utf8)?);
+ for result in describe_record_batch.iter() {
+ match result {
+ Ok(df) => {
+ let record_batch = df.clone().collect().await?;
+ if record_batch.len() == 1 {
Review Comment:
> Thiking, if the code can be simplified a bit, now the code looks overloaded with conditions.
Thanks, I made some changes. If you have time, could you please help with another code 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-datafusion] comphead commented on a diff in pull request #5585: fix dataframe only boolean/binary column got error on describe
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5585:
URL: https://github.com/apache/arrow-datafusion/pull/5585#discussion_r1135713409
##########
datafusion/core/src/dataframe.rs:
##########
@@ -437,19 +416,27 @@ impl DataFrame {
))];
for field in original_schema_fields {
let mut array_datas = vec![];
- for record_batch in describe_record_batch.iter() {
- // safe unwrap since aggregate record batches should have at least 1 record
- let column = record_batch.get(0).unwrap().column_by_name(field.name());
- match column {
- Some(c) => {
- if field.data_type().is_numeric() {
- array_datas.push(cast(c, &DataType::Float64)?);
- } else {
- array_datas.push(cast(c, &DataType::Utf8)?);
+ for result in describe_record_batch.iter() {
+ match result {
+ Ok(df) => {
+ let record_batch = df.clone().collect().await?;
+ if record_batch.len() == 1 {
Review Comment:
Thiking, if the code can be simplified a bit, now the code looks overloaded with conditions.
--
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-datafusion] jiangzhx commented on a diff in pull request #5585: fix dataframe only boolean/binary column got error on describe
Posted by "jiangzhx (via GitHub)" <gi...@apache.org>.
jiangzhx commented on code in PR #5585:
URL: https://github.com/apache/arrow-datafusion/pull/5585#discussion_r1135134980
##########
datafusion/core/src/dataframe.rs:
##########
@@ -437,19 +416,27 @@ impl DataFrame {
))];
for field in original_schema_fields {
let mut array_datas = vec![];
- for record_batch in describe_record_batch.iter() {
- // safe unwrap since aggregate record batches should have at least 1 record
- let column = record_batch.get(0).unwrap().column_by_name(field.name());
- match column {
- Some(c) => {
- if field.data_type().is_numeric() {
- array_datas.push(cast(c, &DataType::Float64)?);
- } else {
- array_datas.push(cast(c, &DataType::Utf8)?);
+ for result in describe_record_batch.iter() {
+ match result {
+ Ok(df) => {
+ let record_batch = df.clone().collect().await?;
+ if record_batch.len() == 1 {
+ if let Some(column) =
+ record_batch.get(0).unwrap().column_by_name(field.name())
+ {
+ if field.data_type().is_numeric() {
+ array_datas.push(cast(column, &DataType::Float64)?);
+ } else {
+ array_datas.push(cast(column, &DataType::Utf8)?);
+ }
+ } else {
+ array_datas
+ .push(Arc::new(StringArray::from_slice(["null"])));
+ }
}
}
- //if None mean the column cannot be min/max aggregation
- None => {
+ Err(_) => {
+ //Handling error when only boolean/binary column, and in other cases
array_datas.push(Arc::new(StringArray::from_slice(["null"])));
Review Comment:
Maybe it's not a good idea to catch all errors and return StringArray::from_slice(["null"]).
--
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