ARROW-7364: [Rust][DataFusion] Add cast options to cast kernel and TRY_CAST to DataFusion by seddonm1 · Pull Request #9682 · apache/arrow

seddonm1 · 2021-03-12T00:30:07Z

This is my WIP implementation of adding CastOptions to the Rust Kernel and changing the default CastOptions for DataFusion to be safe = false.

From here we have two options:

use some sort of feature flag (like Spark) to set the default (see spark.sql.ansi.enabled here).
add a safe_cast expr to do the same operation but override the default.

Thoughts?

github-actions · 2021-03-12T00:30:32Z

https://issues.apache.org/jira/browse/ARROW-7364

andygrove · 2021-03-12T01:29:03Z

rust/arrow/src/compute/kernels/cast.rs

Do we anticipate other cast options in the future?

I think we may. The CPP implementation is here:

arrow/cpp/src/arrow/compute/cast.h

Line 45 in a554ff7

explicit CastOptions(bool safe = true)

and presumably these were added for justifiable reasons. It is always a tricky balance as my personal view is having too many options (like Spark) is way too intimidating.

andygrove · 2021-03-12T01:37:53Z

Thanks @seddonm1 this is a really important feature to add.

To answer your question about the two options, I would favor making this explicit and let the user choose between cast and safe_cast (or whatever name we choose) when building their logical query plan via DataFrame or SQL.

Our optimizer has automatic type coercion rules and I would expect those to default to safe_cast. If the user doesn't like that behavior then they can add explicit unsafe casts instead.

alamb

This is great @seddonm1 -- I really like it. Very nicely done.

Testing

I think we should add at least one tests in the cast.rs kernel that hits each of the invalid cast conditions (where CastError is raised).

Backwards compatibility

Another suggestion about API design is to consider maintaining backwards compatibility with the existing cast kernes. Specifically, by adding cast_options to the cast kernel we will break backwards compatibility. One way to offer a backwards compatible API:

fn safe_cast(...)

// Same semantics as on current main
fn cast(...)

fn cast_with_options(...)

That might avoid having to import DEFAULT_CAST_OPTIONS in several other modules.

Naming / CAST / TRY_CAST

Our optimizer has automatic type coercion rules and I would expect those to default to safe_cast. If the user doesn't like that behavior then they can add explicit unsafe casts instead.

I agree with this statement. I did some googling this morning and did not find anything similar in postgres we could model datafusion on. However, I found that SQL Server has a try_cast function that perhaps we could follow https://dba.stackexchange.com/questions/203934/postgresql-alternative-to-sql-server-s-try-cast-function

alamb · 2021-03-12T10:51:42Z

rust/arrow/src/compute/kernels/cast.rs

+1 for using a separate error variant for CastError

alamb · 2021-03-12T10:53:26Z

rust/arrow/src/compute/kernels/cast.rs

Given the comments say this function should be infallable it probably doesn't need the cast_options parameter

alamb · 2021-03-12T10:56:58Z

rust/arrow/src/compute/kernels/cast.rs

I would suggest a more specific message here and in the other places that CastError is raised. When I used postgres (years ago) it didn't included these details in its error messages, which was heavily frustrating

Something like

ArrowError::CastError(format!("Cannot cast string '{}' to numeric type {}", from.value(i), T::DATA_TYPE))

Agree but i guess we just need to verify that these are not logged as it is easy to leak data.

alamb · 2021-03-12T11:04:37Z

rust/arrow/src/compute/kernels/cast.rs

In the tests I would recommend using explicit cast options with the variant being tested rather than DEFAULT_CAST_OPTIONS so it is clear what is being tested -- the default behavior or the behavior of unsafe casts

alamb · 2021-03-12T11:05:42Z

rust/datafusion/src/physical_plan/expressions/cast.rs

seddonm1 · 2021-03-12T20:05:09Z

Thanks both. I am away this weekend but will incorporate these changes and will notify early next week for final PR with more tests.

alamb · 2021-03-12T21:33:02Z

👍

seddonm1 · 2021-03-15T23:36:16Z

@alamb @andygrove

I have made the changes to the core from your review but I have not built the try_cast vs safe_cast functionality yet.

Here are the two names and their provenance:

SQL Server try_cast
BigQuery safe_cast

My (changed) vote is try_cast for two reasons:

I like referring to another more mainstream SQL dialect (MS SQL has been around a lot longer than BigQuery)
try_cast does feel like a more 'rusty' implementation

Based on the outcome of this vote I will raise a PR against the sqlparser-rs so we can support the same TRY_CAST(x AS y) syntax and then can finish this PR.

alamb · 2021-03-16T21:46:01Z

Thanks @seddonm1

seddonm1 · 2021-03-17T22:41:49Z

FYI apache/datafusion-sqlparser-rs#299 has been raised to add TRY_CAST to the parser.

seddonm1 · 2021-03-25T05:39:21Z

@alamb I think this is ready to have a look at as I would appreciate some feedback.

Thanks to @Dandandan for releasing the new sqlparser-rs version so fast 👍

alamb · 2021-03-26T11:46:24Z

I will check it out later today

…

On Thu, Mar 25, 2021 at 1:39 AM Mike Seddon ***@***.***> wrote: @alamb <https://github.com/alamb> I think this is ready to have a look at as I would appreciate some feedback. Thanks to @Dandandan <https://github.com/Dandandan> for releasing the new sqlparser-rs version so fast 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9682 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXZMKOLPKIMGQPPWDJPYLTFLEBTANCNFSM4ZBJA54Q> .

alamb

I think this looks great @seddonm1 -- it looks good to go (once we get a clean CI run) to me 👍

alamb · 2021-03-26T21:34:11Z

rust/datafusion/src/logical_plan/expr.rs

it may be good to add a note to Cast saying that a runtime error will result if the expression can not be cast.

alamb · 2021-03-26T21:34:47Z

rust/datafusion/src/logical_plan/expr.rs

It seems like given the semantics of TryCast that this should always return true

alamb · 2021-03-26T21:36:51Z

rust/datafusion/src/physical_plan/planner.rs

Doesn't expressions::cast do the same thing? I wonder if we need to change this?

alamb · 2021-03-26T21:38:13Z

rust/datafusion/tests/sql.rs

I don't really understand what the original test was testing 🤔 but the change looks better

This was a hack to create a NULL of UTF8 type (given the previous CAST semantics the failed cast would be NULL)

seddonm1 · 2021-03-26T22:13:36Z

@alamb Thanks for the review. This had a bit of a gap while waiting for the sqlparser changes so I lost context. I will finish it Monday (your comments were absolutely correct) and hopefully then we can merge early in the week.

alamb · 2021-03-27T12:40:47Z

Thanks again @seddonm1

alamb · 2021-04-01T21:19:28Z

@seddonm1 just let me know when this one is ready and we can 🚀

seddonm1 · 2021-04-02T02:11:34Z

rust/datafusion/tests/sql.rs

    let results = collect(plan).await.expect(&msg);

    assert_eq!(logical_schema.as_ref(), optimized_logical_schema.as_ref());
-    assert_eq!(


This is interesting as it assumes none of the attributes can change 'dynamically' at runtime. I.e. if we TRY_CAST a column it can always return a NULL. This does not support runtime evaluation.

seddonm1 · 2021-04-02T02:13:01Z

@alamb I reworked this based on your comments and decided that explicitly creating a cast vs try_cast expression is valuable when reading things like the type_coercion code which now is more obvious which function is being used (try_cast). Can you have another look? (sorry I had to force push due to my doing bad git)

alamb

I think it looks good -- if you think it is good @seddonm1 I'll merge it in. I may take a shot at a follow on PR to optimize the safe_cast == true case

alamb · 2021-04-03T17:49:09Z

rust/arrow/src/compute/kernels/cast.rs

+                }
+            }
+        })
+        .collect::<Result<Vec<Option<_>>>>()?;


I wonder if we need the collect here (and the few places below)? It seems like we might be able to use the iter directly for the case where cast_options.safe == true

I worry that unsafe casts may get slower if we always created a vec for them

This is a good question. I can test removing them.

FYI I tried here: seddonm1#2 and it seems to work well. I am trying to gather some performance benchmarks to see if it matters

ARROW-7364: [Rust] Specialize cast_options.safe casting for performance

alamb · 2021-04-06T13:19:31Z

I am running the tests one more time locally, and if they pass I'll merge this in

alamb · 2021-04-06T13:21:09Z

all looks good 👍 thanks again @seddonm1 !

@andygrove

…Y_CAST to DataFusion @andygrove @alamb @nevi-me This is my WIP implementation of adding `CastOptions` to the Rust Kernel and changing the default `CastOptions` for DataFusion to be `safe = false`. From here we have two options: 1. use some sort of feature flag (like Spark) to set the default (see `spark.sql.ansi.enabled` [here](https://spark.apache.org/docs/latest/configuration.html#runtime-sql-configuration)). 2. add a `safe_cast` `expr` to do the same operation but override the default. Thoughts? Closes apache#9682 from seddonm1/safe-cast Lead-authored-by: Mike Seddon <[email protected]> Co-authored-by: Andrew Lamb <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>

github-actions bot added Component: Rust - DataFusion Component: Rust Component: Parquet labels Mar 12, 2021

seddonm1 marked this pull request as draft March 12, 2021 00:31

andygrove reviewed Mar 12, 2021

View reviewed changes

alamb reviewed Mar 12, 2021

View reviewed changes

seddonm1 mentioned this pull request Mar 21, 2021

Implement TRY_CAST apache/datafusion-sqlparser-rs#299

Merged

seddonm1 force-pushed the safe-cast branch from e6a0ce4 to 5b35afd Compare March 25, 2021 04:32

seddonm1 marked this pull request as ready for review March 25, 2021 05:38

alamb approved these changes Mar 26, 2021

View reviewed changes

add cast_options and try_cast

5a70a6a

seddonm1 force-pushed the safe-cast branch from efc3114 to 5a70a6a Compare April 2, 2021 02:09

seddonm1 commented Apr 2, 2021

View reviewed changes

alamb changed the title ~~ARROW-7364: [Rust] Add cast options to cast kernel [WIP]~~ ARROW-7364: [Rust] Add cast options to cast kernel Apr 3, 2021

alamb changed the title ~~ARROW-7364: [Rust] Add cast options to cast kernel~~ ARROW-7364: [Rust][DataFusion] Add cast options to cast kernel Apr 3, 2021

alamb changed the title ~~ARROW-7364: [Rust][DataFusion] Add cast options to cast kernel~~ ARROW-7364: [Rust][DataFusion] Add cast options to cast kernel and TRY_CAST to DataFusion Apr 3, 2021

alamb approved these changes Apr 3, 2021

View reviewed changes

ARROW-XXX: [Rust] Specialize cast_options.safe casting for performance

1aa23bd

alamb mentioned this pull request Apr 5, 2021

ARROW-7364: [Rust] Specialize cast_options.safe casting for performance seddonm1/arrow#2

Merged

Merge pull request #2 from alamb/alamb/faster_cast

9527cab

ARROW-7364: [Rust] Specialize cast_options.safe casting for performance

alamb closed this in 81f6521 Apr 6, 2021

asfimport mentioned this pull request Apr 6, 2021

[Rust] Add cast options to cast kernel #23641

Closed

Conversation

seddonm1 commented Mar 12, 2021

Uh oh!

github-actions bot commented Mar 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andygrove commented Mar 12, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Testing

Backwards compatibility

Naming / CAST / TRY_CAST

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seddonm1 commented Mar 12, 2021

Uh oh!

alamb commented Mar 12, 2021

Uh oh!

seddonm1 commented Mar 15, 2021

Uh oh!

alamb commented Mar 16, 2021

Uh oh!

seddonm1 commented Mar 17, 2021

Uh oh!

seddonm1 commented Mar 25, 2021

Uh oh!

alamb commented Mar 26, 2021 via email

Uh oh!

alamb left a comment

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!

Choose a reason for hiding this comment

Uh oh!

seddonm1 commented Mar 26, 2021

Uh oh!

alamb commented Mar 27, 2021

Uh oh!

alamb commented Apr 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seddonm1 commented Apr 2, 2021

Uh oh!

alamb left a comment

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 commented Apr 6, 2021

Uh oh!

alamb commented Apr 6, 2021