Lazy array display (#3638) #3647

tustvold · 2023-02-01T19:14:55Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

The output for nested types has been made more consistent, fields are no longer enclosed in " without any escaping, and the same null sentinel is used as elsewhere.

Invalid timezones will result in an error, as opposed to rendering the error to the output string.

Date64 prints out the encoded time, previously it only printed the date which was incorrect.

tustvold · 2023-02-01T19:16:06Z

arrow-cast/src/display.rs

+macro_rules! timestamp_display {
+    ($($t:ty),+) => {
+        $(impl<'a> DisplayIndexState for &'a PrimitiveArray<$t> {
+            type State = Option<Tz>;


This is the motivation for the "deviation" from the proposal in #3638, we can parse the timezone once and use this for formatting

tustvold · 2023-02-01T19:17:26Z

arrow-array/src/array/primitive_array.rs

@@ -1067,7 +1067,7 @@ impl<T: ArrowPrimitiveType> From<ArrayData> for PrimitiveArray<T> {
    }
 }

-impl<T: DecimalType + ArrowPrimitiveType> PrimitiveArray<T> {
+impl<T: DecimalType> PrimitiveArray<T> {


Drive by cleanup

tustvold · 2023-02-01T19:17:49Z

arrow-cast/src/display.rs

+    }
+}
+
+macro_rules! primitive_display {


Unfortunately the lack of trait specialization means we are forced to use macros here

alamb

In general I think this PR looks quite good and make the code cleaner and easier to understand. Thank you so much @tustvold

I didn't quite get through the entire display.rs file yet but what I did get through is quite neat and looks good so far.

I believe this is related to #3638

Date64 prints out the encoded time, previously it only printed the date which was incorrect.

While I think I understand the rationale for this, I think it is worth double checking and maybe considering offering an escape valve formatting option if people want the old behavior.

alamb · 2023-02-07T23:16:26Z

arrow-json/src/writer.rs

+        | DataType::Time32(_)
+        | DataType::Time64(_)
+        | DataType::Duration(_) => {
+            // TODO: Set options


Is this a TODO for this PR or for some future PR? It seems like the default options work well enough for this?

alamb · 2023-02-07T23:19:33Z

arrow-json/src/writer.rs

@@ -937,7 +892,7 @@ mod tests {

        assert_json_eq(
            &buf,
-            r#"{"date32":"2018-11-13","date64":"2018-11-13","name":"a"}
+            r#"{"date32":"2018-11-13","date64":"2018-11-13T17:11:10.011","name":"a"}


Is the idea for this change that since Date64 can repreent milliseconds it should be formatted like a timestamp even thought that may be surprising?

https://github.com/apache/arrow/blob/39bad5442c6447bf07594b09e4b29118b3211460/format/Schema.fbs#L214-L215

I admit the docs aren't very helpful in this respect: https://docs.rs/arrow/32.0.0/arrow/array/type.Date64Array.html

Perhaps Date64 represents a DateTIme? It is a rather strange data type

#3678 <-- I tried to clarify a little

alamb · 2023-02-07T23:19:44Z

arrow-schema/src/datatype.rs

@@ -290,7 +290,7 @@ pub enum IntervalUnit {
 }

 // Sparse or Dense union layouts
-#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Copy)]


alamb · 2023-02-07T23:21:29Z

arrow/src/util/pretty.rs

+            "+---------------------+",
+            "| f                   |",
+            "+---------------------+",
+            "| 2005-03-18T01:58:20 |",


This is the same change as in the json code -- I realize Date64 has precision greater than a day, but I wonder if people will be surprised by this change 🤔

alamb · 2023-02-07T23:25:05Z

parquet/src/arrow/arrow_writer/mod.rs

-            | {"list": [{"leaf_a": 6, "leaf_b": null}, {"leaf_a": 7, "leaf_b": null}, {"leaf_a": 8, "leaf_b": null}, {"leaf_a": 9, "leaf_b": 1}]} |
-            | {"list": [{"leaf_a": 10, "leaf_b": null}]}                                                                                          |
-            +-------------------------------------------------------------------------------------------------------------------------------------+
+            +-------------------------------------------------------------------------------------------------------+


this change certainly looks nicer to me -- I wonder if someone wants the old behavior (which looks like JSON), they should use the JSON writer, correct?

Yes, if someone wants actual JSON, they should use the JSON writer, what this previously produced was quasi-JSON that wouldn't illegal escape characters

alamb · 2023-02-07T23:27:29Z

arrow-cast/src/display.rs

+}
+
+impl<'a> FormatOptions<'a> {
+    /// If set to `true` any formatting errors will be written to the output


alamb · 2023-02-07T23:28:16Z

arrow-cast/src/display.rs

+}
+
+/// Implements [`Display`] for a specific array value
+pub struct ValueFormatter<'a> {


this is quite clever 👏

alamb · 2023-02-07T23:29:16Z

arrow-cast/src/display.rs

+impl<'a> ValueFormatter<'a> {
+    /// Writes this value to the provided [`Write`]
+    ///
+    /// Note: this ignores [`FormatOptions::with_display_error`]


Suggested change

/// Note: this ignores [`FormatOptions::with_display_error`]

/// Note: this ignores [`FormatOptions::with_display_error`] and

/// will return an error on formatting issue

alamb

I finished reading this PR -- it is really nicely done @tustvold 🏅

alamb · 2023-02-07T23:40:09Z

arrow-cast/src/display.rs

+
+macro_rules! decimal_display {
+    ($($t:ty),+) => {
+        $(impl<'a> DisplayIndexState<'a> for &'a PrimitiveArray<$t> {


I am impressed that you got the types to work out with references within macros 🤯

alamb · 2023-02-07T23:43:22Z

arrow-cast/src/display.rs

+/// Get the value at the given row in an array as a String.
+///
+/// Note this function is quite inefficient and is unlikely to be
+/// suitable for converting large arrays or record batches.


Suggested change

/// suitable for converting large arrays or record batches.

/// suitable for converting large arrays or record batches.

///

/// Please see [`ArrayFormatter`] for a better interface

alamb · 2023-02-07T23:44:56Z

arrow-cast/src/display.rs

+}
+
+/// A string formatter for an [`Array`]
+pub struct ArrayFormatter<'a> {


As far as I understand this is the main new interface. I think if we added an example to the docs here it would be great and help this usability a lot. A follow on PR would be fine (and I would be happy to help write it as well)

askoa · 2023-02-08T16:36:05Z

arrow-cast/src/display.rs

+///
+#[derive(Debug, Clone)]
+pub struct FormatOptions<'a> {
+    safe: bool,


There is no comment explaining safe

ursabot · 2023-02-08T17:06:25Z

Benchmark runs are scheduled for baseline = 9b48f34 and contender = a3b344d. a3b344d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the arrow Changes to the arrow crate label Feb 1, 2023

tustvold commented Feb 1, 2023

View reviewed changes

tustvold force-pushed the rework-display branch from 09e620b to 0a87adf Compare February 1, 2023 19:17

tustvold commented Feb 1, 2023

View reviewed changes

tustvold force-pushed the rework-display branch from 0a87adf to 1ad5dc1 Compare February 1, 2023 19:21

This was referenced Feb 2, 2023

Use array_value_to_string in arrow-csv #3514

Merged

Return references from FixedSizeListArray and MapArray #3652

Merged

tustvold force-pushed the rework-display branch from 1ad5dc1 to 790f1c9 Compare February 2, 2023 18:57

tustvold added 7 commits February 3, 2023 12:13

Lazy array display (apache#3638)

17d990b

Update CSV writer

f24b986

Borrow

0a047e9

Time formatting

98dfcde

Update pretty

41c8ff0

Add FixedSizeBinaryArray

630b6ac

Further tweaks

58d8872

tustvold force-pushed the rework-display branch from 790f1c9 to 58d8872 Compare February 3, 2023 18:52

github-actions bot added the parquet Changes to the parquet crate label Feb 3, 2023

tustvold added the api-change Changes to the arrow API label Feb 3, 2023

tustvold added 4 commits February 3, 2023 18:57

Clippy

f9605dd

More clippy

404d37b

More tweaks

9ce362b

More clippy

fba93d5

tustvold force-pushed the rework-display branch from d9afe59 to fba93d5 Compare February 3, 2023 19:28

tustvold added 2 commits February 4, 2023 09:09

Clippy

bee36af

Use lexical_core

cede615

This was referenced Feb 4, 2023

Implement std::fmt::Write for StringBuilder (#3638) #3659

Merged

Use ArrayFormatter in Cast Kernel #3668

Merged

Update doctest

e4e8387

tustvold marked this pull request as ready for review February 6, 2023 17:26

alamb reviewed Feb 7, 2023

View reviewed changes

alamb approved these changes Feb 7, 2023

View reviewed changes

tustvold added 3 commits February 8, 2023 14:38

Review feedback

6d6a230

Merge remote-tracking branch 'upstream/master' into rework-display

4562430

Bump CI

8cf3c97

askoa reviewed Feb 8, 2023

View reviewed changes

Review feedback

bd82da1

tustvold merged commit a3b344d into apache:master Feb 8, 2023

alamb mentioned this pull request Feb 8, 2023

Minor: Add some examples to Date*Array and Time*Array #3678

Merged

This was referenced Feb 22, 2023

Impl Parser for more types in arrow_array::types #3735

Closed

display NULL instead of empty string #2476

Closed

Array --> Display formatter that supports more options and is configurable #3638

Closed

tustvold mentioned this pull request Mar 16, 2023

Implement Debug printing "kernel" #199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy array display (#3638) #3647

Lazy array display (#3638) #3647

tustvold commented Feb 1, 2023 •

edited

Loading

tustvold Feb 1, 2023

tustvold Feb 1, 2023

tustvold Feb 1, 2023

alamb left a comment

alamb Feb 7, 2023

alamb Feb 7, 2023

tustvold Feb 8, 2023

alamb Feb 8, 2023

alamb Feb 7, 2023

alamb Feb 7, 2023

alamb Feb 7, 2023

tustvold Feb 8, 2023

alamb Feb 7, 2023

alamb Feb 7, 2023

alamb Feb 7, 2023

alamb left a comment

alamb Feb 7, 2023

alamb Feb 7, 2023

alamb Feb 7, 2023

askoa Feb 8, 2023

ursabot commented Feb 8, 2023

	/// Note: this ignores [`FormatOptions::with_display_error`]
	/// Note: this ignores [`FormatOptions::with_display_error`] and
	/// will return an error on formatting issue

Lazy array display (#3638) #3647

Lazy array display (#3638) #3647

Conversation

tustvold commented Feb 1, 2023 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ursabot commented Feb 8, 2023

tustvold commented Feb 1, 2023 •

edited

Loading