From 0bf527a1f6419ff99903b5f2104e98a0771dc727 Mon Sep 17 00:00:00 2001 From: jayzhan211 Date: Sat, 18 Nov 2023 17:44:00 +0800 Subject: [PATCH 1/3] fix display for list Signed-off-by: jayzhan211 --- datafusion/common/src/scalar.rs | 28 +++++++++++++------ .../sqllogictest/test_files/explain.slt | 15 ++++++++++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/datafusion/common/src/scalar.rs b/datafusion/common/src/scalar.rs index fd1ceb5fad78..d0714336acc7 100644 --- a/datafusion/common/src/scalar.rs +++ b/datafusion/common/src/scalar.rs @@ -34,6 +34,7 @@ use crate::utils::array_into_list_array; use arrow::buffer::{NullBuffer, OffsetBuffer}; use arrow::compute::kernels::numeric::*; use arrow::datatypes::{i256, Fields, SchemaBuilder}; +use arrow::util::display::{ArrayFormatter, FormatOptions}; use arrow::{ array::*, compute::kernels::cast::{cast_with_options, CastOptions}, @@ -2931,12 +2932,21 @@ impl fmt::Display for ScalarValue { )?, None => write!(f, "NULL")?, }, - ScalarValue::List(arr) | ScalarValue::FixedSizeList(arr) => write!( - f, - "{}", - arrow::util::pretty::pretty_format_columns("col", &[arr.to_owned()]) - .unwrap() - )?, + ScalarValue::List(arr) | ScalarValue::FixedSizeList(arr) => { + // TODO: Remove NullArray + // There are NullArray(0) currently, so we need to check the length + if arr.len() == 0 { + write!(f, "[]")? + } else { + let options = FormatOptions::default().with_display_error(true); + // TODO: No way to map to fmt::Error yet + let formatter = ArrayFormatter::try_new(arr, &options) + .map_err(DataFusionError::ArrowError) + .unwrap(); + let value_formatter = formatter.value(0); + write!(f, "{value_formatter}")? + } + } ScalarValue::Date32(e) => format_option!(f, e)?, ScalarValue::Date64(e) => format_option!(f, e)?, ScalarValue::Time32Second(e) => format_option!(f, e)?, @@ -3011,8 +3021,10 @@ impl fmt::Debug for ScalarValue { } ScalarValue::LargeBinary(None) => write!(f, "LargeBinary({self})"), ScalarValue::LargeBinary(Some(_)) => write!(f, "LargeBinary(\"{self}\")"), - ScalarValue::FixedSizeList(arr) => write!(f, "FixedSizeList([{arr:?}])"), - ScalarValue::List(arr) => write!(f, "List([{arr:?}])"), + ScalarValue::FixedSizeList(_) => write!(f, "FixedSizeList({self})"), + ScalarValue::List(_) => { + write!(f, "List({self})") + } ScalarValue::Date32(_) => write!(f, "Date32(\"{self}\")"), ScalarValue::Date64(_) => write!(f, "Date64(\"{self}\")"), ScalarValue::Time32Second(_) => write!(f, "Time32Second(\"{self}\")"), diff --git a/datafusion/sqllogictest/test_files/explain.slt b/datafusion/sqllogictest/test_files/explain.slt index 129814767ca2..634e7e7fbdd9 100644 --- a/datafusion/sqllogictest/test_files/explain.slt +++ b/datafusion/sqllogictest/test_files/explain.slt @@ -362,3 +362,18 @@ GlobalLimitExec: skip=0, fetch=10, statistics=[Rows=Exact(8), Bytes=Absent, [(Co statement ok set datafusion.execution.collect_statistics = false; + +# Explain ArrayFuncions + +statement ok +set datafusion.explain.physical_plan_only = false + +query TT +explain select make_array(make_array(1, 2, 3), make_array(4, 5, 6)); +---- +logical_plan +Projection: List([[1, 2, 3], [4, 5, 6]]) AS make_array(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5),Int64(6))) +--EmptyRelation +physical_plan +ProjectionExec: expr=[[[1, 2, 3], [4, 5, 6]] as make_array(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5),Int64(6)))] +--EmptyExec: produce_one_row=true From 716977b4991b2c4d091cc699e35cc86d01b2b42c Mon Sep 17 00:00:00 2001 From: jayzhan211 Date: Sat, 18 Nov 2023 20:43:41 +0800 Subject: [PATCH 2/3] fmt Signed-off-by: jayzhan211 --- datafusion/common/src/scalar.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/common/src/scalar.rs b/datafusion/common/src/scalar.rs index d0714336acc7..2f0656c21082 100644 --- a/datafusion/common/src/scalar.rs +++ b/datafusion/common/src/scalar.rs @@ -2936,7 +2936,7 @@ impl fmt::Display for ScalarValue { // TODO: Remove NullArray // There are NullArray(0) currently, so we need to check the length if arr.len() == 0 { - write!(f, "[]")? + write!(f, "[]")? } else { let options = FormatOptions::default().with_display_error(true); // TODO: No way to map to fmt::Error yet From 6727b924a17c2fcf88da85bc268766523331f951 Mon Sep 17 00:00:00 2001 From: jayzhan211 Date: Wed, 22 Nov 2023 07:57:54 +0800 Subject: [PATCH 3/3] address comment Signed-off-by: jayzhan211 --- datafusion/common/src/scalar.rs | 19 ++++++------------- .../sqllogictest/test_files/explain.slt | 10 ++++++++++ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/datafusion/common/src/scalar.rs b/datafusion/common/src/scalar.rs index 2f0656c21082..21cd50dea8c7 100644 --- a/datafusion/common/src/scalar.rs +++ b/datafusion/common/src/scalar.rs @@ -2933,19 +2933,12 @@ impl fmt::Display for ScalarValue { None => write!(f, "NULL")?, }, ScalarValue::List(arr) | ScalarValue::FixedSizeList(arr) => { - // TODO: Remove NullArray - // There are NullArray(0) currently, so we need to check the length - if arr.len() == 0 { - write!(f, "[]")? - } else { - let options = FormatOptions::default().with_display_error(true); - // TODO: No way to map to fmt::Error yet - let formatter = ArrayFormatter::try_new(arr, &options) - .map_err(DataFusionError::ArrowError) - .unwrap(); - let value_formatter = formatter.value(0); - write!(f, "{value_formatter}")? - } + // ScalarValue List should always have a single element + assert_eq!(arr.len(), 1); + let options = FormatOptions::default().with_display_error(true); + let formatter = ArrayFormatter::try_new(arr, &options).unwrap(); + let value_formatter = formatter.value(0); + write!(f, "{value_formatter}")? } ScalarValue::Date32(e) => format_option!(f, e)?, ScalarValue::Date64(e) => format_option!(f, e)?, diff --git a/datafusion/sqllogictest/test_files/explain.slt b/datafusion/sqllogictest/test_files/explain.slt index 634e7e7fbdd9..c8eff2f301aa 100644 --- a/datafusion/sqllogictest/test_files/explain.slt +++ b/datafusion/sqllogictest/test_files/explain.slt @@ -377,3 +377,13 @@ Projection: List([[1, 2, 3], [4, 5, 6]]) AS make_array(make_array(Int64(1),Int64 physical_plan ProjectionExec: expr=[[[1, 2, 3], [4, 5, 6]] as make_array(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5),Int64(6)))] --EmptyExec: produce_one_row=true + +query TT +explain select [[1, 2, 3], [4, 5, 6]]; +---- +logical_plan +Projection: List([[1, 2, 3], [4, 5, 6]]) AS make_array(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5),Int64(6))) +--EmptyRelation +physical_plan +ProjectionExec: expr=[[[1, 2, 3], [4, 5, 6]] as make_array(make_array(Int64(1),Int64(2),Int64(3)),make_array(Int64(4),Int64(5),Int64(6)))] +--EmptyExec: produce_one_row=true