ARROW-10165: [Rust] [DataFusion]: Remove special case DataFusion casting checks in favor of Arrow cast kernel #8340

alamb · 2020-10-05T10:50:50Z

This PR removes the explicit plan-time cast support check from DataFusion and instead uses the runtime check of the arrow compute cast kernel.

The upside of this change is less code overall and increased casting support (DataFusion now supports all casting supported by Arrow rather than just the subset that was hard coded in can_coerce_from).

A potential downside is that an unsupported cast will produce an error later in the process (after the plan has started running rather than during plan time).

This PR is part of a set of changes to support DictionaryArrays in DataFusion. Once I add the appropriate support to the cast kernel, DataFusion will be able to cast DictionaryArray types correctly with no additional code.

Prior to this change, the test in sql failed with this error message:

---- query_on_string_dictionary stdout ----
thread 'query_on_string_dictionary' panicked at 'called `Result::unwrap()` on an `Err` value: General("\'Dictionary(Int32, Utf8) = Utf8\' can\'t be evaluated because there isn\'t a common type to coerce the types to")', datafusion/tests/sql.rs:925:16
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

github-actions · 2020-10-05T11:05:46Z

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

alamb · 2020-10-05T11:02:43Z

rust/datafusion/tests/sql.rs

 /// `result[row][column]`
 async fn execute(ctx: &mut ExecutionContext, sql: &str) -> Vec<Vec<String>> {
-    let plan = ctx.create_logical_plan(&sql).unwrap();
+    let msg = format!("Creating logical plan for '{}'", sql);


These are some changes to improve the test error reporting (rather than a straight up panic, some diagnostic information is printed as well)

alamb · 2020-10-05T11:04:08Z

rust/datafusion/tests/sql.rs

 }
+
+#[tokio::test]
+async fn query_on_string_dictionary() -> Result<()> {


Here is the end to end testcast I am working on for DictionaryArray support -- with this PR we can do basic filtering in DataFusion. There are a few more PRs needed to complete expressions and aggregation, but they are coming.

alamb · 2020-10-05T11:04:35Z

rust/datafusion/tests/sql.rs

+
+    // The following queries are not yet supported
+
+    // // filtering with constant


I have PRs in the works to support these cases and I will uncomment them as I do so.

…ng checks in favor of Arrow cast kernel

jorgecarleitao · 2020-10-07T03:01:41Z

rust/datafusion/src/logical_plan/mod.rs

    /// # Errors
    ///
-    /// This function errors when it is impossible to cast the expression to the target [arrow::datatypes::DataType].
+    /// Currently no errors happen at plan time. If it is impossible


I am unsure on whether this is advisable:

Doesn't this mean that the plan can fail arbitrarily when a user performs an impossible cast? This can happen like 10hs after the execution starts, when the query finally reaches that point.

I though that the point of having the types available during planning, both logical and physical, was to perform exactly these types of checks. Removing this check seems a regression to me, on which an impossible cast will only be caught during execution.

Isn't it possible to expand can_coerce_from to cover the cases that DataFusion is missing but that arrow cast kernel supports?

It is interesting that there appear to be no uses (or tests) for this function -- https://github.com/apache/arrow/search?q=cast_to I found this quite confusing

@jorgecarleitao what do you think about simply removing the whole thing entirely?

I am sorry, I was confused: can_coerce_from checks whether we can perform a lossless cast. This is different from the ability to perform any cast.

Therefore, I think that the logic we may want is:

the Expr::Cast operation should check that we can perform the cast at all (e.g. a ListArray to a UInt32Array should not be allowed). This should match the casts that the kernel supports.

implicit casts, such as the ones that we perform when we pass arguments to functions to try to match the signature, should use can_coerce_from, as we do not want to perform lossless implicit casts.

AFAIK casts to and from dictionaries are lossless, and thus they can happen at any point in the physical planning. Since all arrays support dictionaries, I guess we only have to error if the implementation is still not available.

What do you think, @alamb ?

@jorgecarleitao -- I think this makes sense. I was confused about the intent of can_coerce_from (aka that it is for lossless conversions only). With this explanation let me go back and give adding dictionary coercion / cast a second pass (though likely not until tomorrow, Thursday, US Eastern time)

@jorgecarleitao -- here is a proposed alternate approach: #8400

alamb · 2020-10-07T10:07:49Z

@jorgecarleitao

Doesn't this mean that the plan can fail arbitrarily when a user performs an impossible cast? This can happen like 10hs after the execution starts, when the query finally reaches that point.

Yes I think that is accurate.

Isn't it possible to expand can_coerce_from to cover the cases that DataFusion is missing but that arrow cast kernel supports?

Yes I can do that -- my rationale for this proposal was to avoiding having to keep two lists of valid typecasts in sync. However, the value of faster errors is a valid point. I will make a new PR to do as you suggest and expand the coverage in can_coerce_from

alamb · 2020-10-06T16:40:45Z

rust/datafusion/src/logical_plan/mod.rs

        let this_type = self.get_type(schema)?;
        if this_type == *cast_to_type {
            Ok(self.clone())
-        } else if can_coerce_from(cast_to_type, &this_type) {


The PR itself is pretty simple -- it just deletes code :)

alamb · 2020-10-07T18:28:09Z

Alternate approach is in #8359

@jorgecarleitao

The code / comments for type coercion are a little confusing and don't make the distinction between coercion and casting clear – this PR attempts to clarify the intent, channeling the information from @jorgecarleitao here: #8340 (comment) Closes #8399 from alamb/alamb/coercion-docs Authored-by: alamb <[email protected]> Signed-off-by: Jorge C. Leitao <[email protected]>

@nevi-me

…ataFusion This is a PR incorporating the feedback from @nevi-me and @jorgecarleitao from #8400 It adds 1. a `can_cast_types` function to the Arrow cast kernel (as suggested by @jorgecarleitao / @nevi-me in #8400 (comment)) that encodes the valid type casting 2. A test that ensures `can_cast_types` and `cast` remain in sync 3. Bug fixes that the test above uncovered (I'll comment inline) 4. Change DataFuson to use `can_cast_types` so that it plans casting consistently with what arrow allows Previously the notions of coercion and casting were somewhat conflated in DataFusion. I have tried to clarify them in #8399 and this PR. See also #8340 (comment) for more discussion. I am adding this functionality so DataFusion gains rudimentary support `DictionaryArray`. Codewise, I am concerned about the duplication in logic between the match statements in `cast` and `can_cast_types. I have some thoughts on how to unify them (see #8400 (comment)), but I don't have time to implement that as it is a bigger change. I think this approach with some duplication is ok, and the test will ensure they remain in sync. Closes #8460 from alamb/alamb/ARROW-10236-casting-rules-2 Authored-by: alamb <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

@nevi-me

…ataFusion This is a PR incorporating the feedback from @nevi-me and @jorgecarleitao from #8400 It adds 1. a `can_cast_types` function to the Arrow cast kernel (as suggested by @jorgecarleitao / @nevi-me in #8400 (comment)) that encodes the valid type casting 2. A test that ensures `can_cast_types` and `cast` remain in sync 3. Bug fixes that the test above uncovered (I'll comment inline) 4. Change DataFuson to use `can_cast_types` so that it plans casting consistently with what arrow allows Previously the notions of coercion and casting were somewhat conflated in DataFusion. I have tried to clarify them in #8399 and this PR. See also #8340 (comment) for more discussion. I am adding this functionality so DataFusion gains rudimentary support `DictionaryArray`. Codewise, I am concerned about the duplication in logic between the match statements in `cast` and `can_cast_types. I have some thoughts on how to unify them (see #8400 (comment)), but I don't have time to implement that as it is a bigger change. I think this approach with some duplication is ok, and the test will ensure they remain in sync. Closes #8460 from alamb/alamb/ARROW-10236-casting-rules-2 Authored-by: alamb <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

@nevi-me

…ataFusion This is a PR incorporating the feedback from @nevi-me and @jorgecarleitao from apache/arrow#8400 It adds 1. a `can_cast_types` function to the Arrow cast kernel (as suggested by @jorgecarleitao / @nevi-me in apache/arrow#8400 (comment)) that encodes the valid type casting 2. A test that ensures `can_cast_types` and `cast` remain in sync 3. Bug fixes that the test above uncovered (I'll comment inline) 4. Change DataFuson to use `can_cast_types` so that it plans casting consistently with what arrow allows Previously the notions of coercion and casting were somewhat conflated in DataFusion. I have tried to clarify them in apache/arrow#8399 and this PR. See also apache/arrow#8340 (comment) for more discussion. I am adding this functionality so DataFusion gains rudimentary support `DictionaryArray`. Codewise, I am concerned about the duplication in logic between the match statements in `cast` and `can_cast_types. I have some thoughts on how to unify them (see apache/arrow#8400 (comment)), but I don't have time to implement that as it is a bigger change. I think this approach with some duplication is ok, and the test will ensure they remain in sync. Closes #8460 from alamb/alamb/ARROW-10236-casting-rules-2 Authored-by: alamb <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

alamb commented Oct 5, 2020

View reviewed changes

andygrove added Component: Rust Component: Rust - DataFusion labels Oct 5, 2020

This was referenced Oct 5, 2020

ARROW-10164: [Rust] Add support for DictionaryArray to cast kernel #8345

Closed

ARROW-10164: [Rust] Add support for DictionaryArray to cast kernel #8346

Closed

ARROW-10163: [Rust] [DataFusion] Add DictionaryArray coercion support to physical plans #8359

Closed

ARROW-10165: [Rust][DataFusion]: Remove special case DataFusion casti…

1885319

…ng checks in favor of Arrow cast kernel

alamb force-pushed the alamb/ARROW-10165-coercion-support branch from 23e2673 to 1885319 Compare October 6, 2020 16:38

alamb marked this pull request as ready for review October 6, 2020 16:40

nevi-me approved these changes Oct 7, 2020

View reviewed changes

jorgecarleitao reviewed Oct 7, 2020

View reviewed changes

alamb closed this Oct 7, 2020

alamb commented Oct 7, 2020

View reviewed changes

alamb mentioned this pull request Oct 8, 2020

ARROW-10235: [Rust][DataFusion] Improve documentation for type coercion #8399

Closed

alamb deleted the alamb/ARROW-10165-coercion-support branch October 8, 2020 14:06

This was referenced Oct 8, 2020

ARROW-10236: [Rust][DataFusion] Unify type casting logic in DataFusion #8400

Closed

ARROW-10236: [Rust] Add can_cast_types to arrow cast kernel, use in DataFusion #8460

Closed

asfimport mentioned this pull request Oct 16, 2020

[Rust] [DataFusion] Make DataFusion casting rules consistent with cast kernel #26234

Closed


		// The following queries are not yet supported

		// // filtering with constant

ARROW-10165: [Rust] [DataFusion]: Remove special case DataFusion casting checks in favor of Arrow cast kernel #8340

ARROW-10165: [Rust] [DataFusion]: Remove special case DataFusion casting checks in favor of Arrow cast kernel #8340

Uh oh!

Conversation

alamb commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 5, 2020

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!

jorgecarleitao Oct 7, 2020 • 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 commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alamb commented Oct 5, 2020 •

edited

Loading

jorgecarleitao Oct 7, 2020 •

edited

Loading