[arrow-cast] Support cast from Numeric (`Int`, `UInt`, etc) to `Utf8View` by tlm365 · Pull Request #6719 · apache/arrow-rs

tlm365 · 2024-11-12T01:36:03Z

Which issue does this PR close?

Closes #6714.
related to #6373

Rationale for this change

Add support cast from numeric(Int/Float/Decimal) to string view (Utf8View).

What changes are included in this PR?

The cast logic and corresponding unit tests.

Are there any user-facing changes?

No.

Signed-off-by: Tai Le Manh <[email protected]>

tlm365 · 2024-11-12T01:48:44Z

@Omega359 During the code review, I found that it is possible to implement support cast for all numeric values (Int/Float/Decimal) to string view. I thought it would be good to implement it in 1 PR instead of creating new issue for each datatype -> so I implemented it in this PR. How do you think about it? If it fine, could you please update the issue description?

tustvold · 2024-11-12T09:28:32Z

arrow-cast/src/cast/string.rs

+    let nulls = array.nulls();
+    for i in 0..array.len() {
+        match nulls.map(|x| x.is_null(i)).unwrap_or_default() {
+            false => builder.append_value(formatter.value(i).try_to_string()?),


It would be more optimal to use the std::fmt::write support as for StringArray above.

As written this will allocate for every value which will be very expensive

@tustvold Thanks so much for reviewing.

It would be more optimal to use the std::fmt::write support as for StringArray above.
As written this will allocate for every value which will be very expensive

I get it now. Will try to implement it. TYSM ❤️

Signed-off-by: Tai Le Manh <[email protected]>

Omega359 · 2024-11-14T18:57:30Z

arrow-cast/src/cast/mod.rs

        (BinaryView, _) => Err(ArrowError::CastError(format!(
            "Casting from {from_type:?} to {to_type:?} not supported",
        ))),
+        (from_type, Utf8View) if from_type.is_primitive() => {


I believe this also fixes the Timestamp -> Utf8View issue. It would be good to have tests for temporal -> Utf8View added to cover this case.

After reviewing the code, I realized that the Timestamp -> Utf8View cast is not supported yet.

The main issue comes from the current implementation of formatter.format.write (source) which currently only applies to DisplayIndex derives (source), but the Temporal datatype is implemented based on DisplayIndexState (source).

I think this issue deserves a separate PR to handle the temporal -> string view casting.

I'll file another PR today to cover the temporal -> Utf8View case unless someone beats me to it.

Signed-off-by: Tai Le Manh <[email protected]>

alamb

Looks great to me -- thank you @tlm365 , @Omega359 and @tustvold

alamb · 2024-11-22T19:27:07Z

arrow-array/src/builder/generic_bytes_view_builder.rs

 /// ```
 pub type StringViewBuilder = GenericByteViewBuilder<StringViewType>;

+impl std::fmt::Write for StringViewBuilder {


this is also what is contemplated by #6373 (aka I think this PR fixes that ticket as well)

arrow-cast/src/cast/mod.rs

…ing_view

alamb

I think there is a problem with the Write implementation.

Here is a PR showing the problem: tlm365#1

alamb · 2024-11-22T19:40:01Z

arrow-array/src/builder/generic_bytes_view_builder.rs


+impl std::fmt::Write for StringViewBuilder {
+    fn write_str(&mut self, s: &str) -> std::fmt::Result {
+        self.append_value(s);


I was writing some tests for this, and it turns out this is different behavior than StringViewBuilder

https://docs.rs/arrow/latest/arrow/array/builder/type.GenericStringBuilder.html#example-incrementally-writing-strings-with-stdfmtwrite

Specifically, calling write_str doesn't compete the row 🤔

I made a PR showing the problem: tlm365#1

I am working on a potential solution so we can unblock this PR

alamb · 2024-11-22T21:54:22Z

Here is proposal for how to implement std::fmt::Write: #6777

Add documentation examples for `StringViewBuilder::write_str`

alamb · 2024-11-27T11:02:18Z

arrow-cast/src/cast/string.rs

+pub(crate) fn value_to_string_view(
+    array: &dyn Array,
+    options: &CastOptions,
+) -> Result<ArrayRef, ArrowError> {
+    let mut builder = StringViewBuilder::with_capacity(array.len());
+    let formatter = ArrayFormatter::try_new(array, &options.format_options)?;
+    let nulls = array.nulls();
+    for i in 0..array.len() {
+        match nulls.map(|x| x.is_null(i)).unwrap_or_default() {
+            true => builder.append_null(),
+            false => formatter.value(i).write(&mut builder)?,
+        }
+    }
+    Ok(Arc::new(builder.finish()))
+}


I have been dreaming about this PR and how to unblock it.

I think it is currently stalled by trying to figure out how to handle std::fmt::Write

Here is an alternate proposal:

Change this implementation to avoid reallocating on each row, but still copy

Remove the std::fmt::Write implementation (and we can sort that out in a different PR)

So the first point might look something like this:

Suggested change

pub(crate) fn value_to_string_view(

array: &dyn Array,

options: &CastOptions,

) -> Result<ArrayRef, ArrowError> {

let mut builder = StringViewBuilder::with_capacity(array.len());

let formatter = ArrayFormatter::try_new(array, &options.format_options)?;

let nulls = array.nulls();

for i in 0..array.len() {

match nulls.map(|x| x.is_null(i)).unwrap_or_default() {

true => builder.append_null(),

false => formatter.value(i).write(&mut builder)?,

}

}

Ok(Arc::new(builder.finish()))

}

pub(crate) fn value_to_string_view(

array: &dyn Array,

options: &CastOptions,

) -> Result<ArrayRef, ArrowError> {

let mut builder = StringViewBuilder::with_capacity(array.len());

let formatter = ArrayFormatter::try_new(array, &options.format_options)?;

let nulls = array.nulls();

// buffer to avoid reallocating on each value

// TODO: replace with write to builder after https://github.com/apache/arrow-rs/issues/6373

mut buffer = String::new();

for i in 0..array.len() {

match nulls.map(|x| x.is_null(i)).unwrap_or_default() {

true => builder.append_null(),

false => {

// write to buffer first and then copy into target array

buffer.clear();

formatter.value(i).write(&mut buffer)?,

bulder.append_value(&buffer)

}

}

}

Ok(Arc::new(builder.finish()))

}

This will work nicely (well, once it compiles) and would unblock downstream issues. 👍🏻

I am trying to work down my review queue / reduce the number of things I am trying to keep track of and we have also gone around too much with this PR on you @tlm365 -- for that I apoloigize

Here is a PR hopefully that will solve #6714:

[arrow-cast] Support cast numeric to string view (alternate) #6816

tlm365 · 2024-12-02T08:04:34Z

Oops, so sorry everyone for the late update 🥹

I am trying to work down my review queue / reduce the number of things I am trying to keep track of and we have also gone around too much with this PR on you @tlm365 -- for that I apologize

I should apologize, thank you for completing it @alamb ❤️.

alamb · 2024-12-02T14:12:28Z

Let's call it a good team effort!

[arrow-cast] Support cast numeric to string view

205e40d

Signed-off-by: Tai Le Manh <[email protected]>

github-actions bot added the arrow Changes to the arrow crate label Nov 12, 2024

fix fmt error

2a937fe

tlm365 force-pushed the cast_numeric_to_string_view branch from f4dfcda to 2a937fe Compare November 12, 2024 02:05

tustvold reviewed Nov 12, 2024

View reviewed changes

Implement std::fmt::Write for StringViewBuilder

2f8f9c3

Signed-off-by: Tai Le Manh <[email protected]>

Omega359 mentioned this pull request Nov 14, 2024

feat: Add stringview support to encode and decode and bit_length apache/datafusion#13332

Merged

Omega359 reviewed Nov 14, 2024

View reviewed changes

tlm365 marked this pull request as draft November 15, 2024 01:11

Update that current impl support only for numeric -> string view

74de9bc

Signed-off-by: Tai Le Manh <[email protected]>

tlm365 force-pushed the cast_numeric_to_string_view branch from 1a6868a to 74de9bc Compare November 15, 2024 05:21

Omega359 mentioned this pull request Nov 15, 2024

Support Temporal -> Utf8View casting #6734

Closed

tlm365 marked this pull request as ready for review November 16, 2024 02:12

Merge branch 'master' into cast_numeric_to_string_view

cdc16c9

alamb changed the title ~~[arrow-cast] Support cast numeric to string view~~ [arrow-cast] Support cast from Decimal (numeric) to Utf8View Nov 22, 2024

alamb changed the title ~~[arrow-cast] Support cast from Decimal (numeric) to Utf8View~~ [arrow-cast] Support cast from Numeric (Int, UInt, etc) to Utf8View` Nov 22, 2024

alamb approved these changes Nov 22, 2024

View reviewed changes

alamb reviewed Nov 22, 2024

View reviewed changes

arrow-cast/src/cast/mod.rs Show resolved Hide resolved

alamb added 3 commits November 22, 2024 14:32

Update arrow-cast/src/cast/mod.rs

d65e228

Merge remote-tracking branch 'apache/master' into cast_numeric_to_str…

2ea0b51

…ing_view

Add documentation examples for StringViewBuilder::write_str

b907827

alamb mentioned this pull request Nov 22, 2024

Add documentation examples for StringViewBuilder::write_str tlm365/arrow-rs#1

Merged

alamb requested changes Nov 22, 2024

View reviewed changes

This was referenced Nov 22, 2024

Implement std::fmt::Write for StringViewBuilder to permit non contiguous data to be written #6373

Open

impl std::fmt::Write for StringViewBuilder / BinaryViewBuilder #6777

Closed

alamb changed the title ~~[arrow-cast] Support cast from Numeric (Int, UInt, etc) to Utf8View`~~ [arrow-cast] Support cast from Numeric (Int, UInt, etc) to Utf8View Nov 22, 2024

Merge pull request #1 from alamb/alamb/document_fmt_write

92732ea

Add documentation examples for `StringViewBuilder::write_str`

tlm365 marked this pull request as draft November 23, 2024 01:30

alamb reviewed Nov 27, 2024

View reviewed changes

alamb mentioned this pull request Nov 29, 2024

[arrow-cast] Support cast numeric to string view (alternate) #6816

Merged

tustvold closed this in #6816 Dec 1, 2024

Comments

Conversation

tlm365 commented Nov 12, 2024 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

tlm365 commented Nov 12, 2024

Uh oh!

tustvold Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Nov 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlm365 commented Dec 2, 2024

Uh oh!

alamb commented Dec 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlm365 commented Nov 12, 2024 •

edited by alamb

Loading

tustvold Nov 12, 2024 •

edited

Loading

alamb Nov 22, 2024 •

edited

Loading