ARROW-10330: [Rust][DataFusion] Implement NULLIF() SQL function #8688

velvia · 2020-11-17T00:26:20Z

This PR implements the NULLIF() SQL function in DataFusion. It is implemented as a BuiltInScalarFunction, with a boolean kernel at the core which creates a new array with a modified null bitmap from the original array, based on the result of a boolean expression. When an input data item is equal to the right side in NULLIF(), then the item's nullity becomes set in the output array.

velvia · 2020-11-17T00:27:56Z

@andygrove @nevi-me would love to hear your feedback on this.... this addition has been useful to us internally.

github-actions · 2020-11-17T00:54:53Z

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

jorgecarleitao

I went carefully through this, and it is really good. Thanks a lot @velvia.

I left some comments that IMO make the PR even more generic.

Would it be possible to update the section SQL Support on the README with this?

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

rust/datafusion/src/physical_plan/expressions.rs

rust/datafusion/tests/sql.rs

vertexclique · 2020-11-17T10:47:13Z

rust/datafusion/src/physical_plan/expressions.rs

+pub static SUPPORTED_NULLIF_TYPES: &'static [DataType] = &[
+    DataType::Boolean,
+    DataType::UInt8,
+    DataType::UInt16,
+    DataType::UInt32,
+    DataType::UInt64,
+    DataType::Int8,
+    DataType::Int16,
+    DataType::Int32,
+    DataType::Int64,
+    DataType::Float32,
+    DataType::Float64,
+];


Better to make these trait bounds with a good comment about how these are selected. I didn't understand:

/// The order of these types correspond to the order on which coercion applies

Can you explain it?

Better to make these trait bounds with a good comment about how these are selected

AFAIK these cannot be trait bounds because logical and physical planning is dynamically typed.

In this case, this is enumerating all valid types that can be (dynamically) passed to the function. If someone tries to call this function with e.g. a ListArray, the logical planner will error with a description that this function does not support that type.

The order here matters because when a function is planned to be called with type X that is not supported by the function, the physical planner will try to (lossless) cast that type to a valid type for that functions, and it does so in the order of this array. In general these should be ordered from fastest to slowest (in the eyes of the implementation), so that the cast chooses the type with the fastest implementation.

Oh, I see, in our private project at work, I have used type algebra definitions to not do these. For now, this can go like how it is, but later I can open a type algebra pr to convert all these to castability.

That is interesting. I would be interested in knowing what is the issue with the current implementation and why type algebra definitions should be used instead. Could you first introduce a proposal with the design e.g. on a google docs, before the PR? In DataFusion we have been doing that for larger changes to avoid committing to an implementation before some general agreement.

Oh definitely will do, give me some time to wrap my head up.

jorgecarleitao

Thanks for your patience, @velvia.

I went through this more carefully, and I think that we have two issues:

the null count of the returned array may not match the number of bits
there may be some issue with arrays with offset != 0.

I left some comments in the code trying to describe what they are and how I tested them.

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

Co-authored-by: Jorge Leitao <[email protected]>

velvia · 2020-11-23T22:01:30Z

@jorgecarleitao and others:

All outstanding issues should be resolved now, including the nonzero array offsets
Rebased to latest master as of today
I did not make the nullif kernel more generic, the reason is that I took @vertexclique 's suggestion to slice the left array data buffer, which allows us to work with the shifted/0-based null combined bitmaps. The result of this is that we must assume that the data buffers are primitive, otherwise shifting/slicing the data buffers might not work for non primitive types or be more complex. Let me know what you think.
The alternative is to re-shift the combined bitmaps back to left.offset(), which might have space implications, but would be more generic (would not require any change of the left array's data buffers), and would have to wait for the other PR as well most likely.... I personally would prefer not to wait :)

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

jorgecarleitao

Thanks a lot for the effort. LGTM

It is fine to remain for primitive types for now.

velvia · 2020-11-27T20:09:40Z

How does it get merged from here? Sorry unclear on the process, thanks!

jorgecarleitao · 2020-11-27T20:13:01Z

@velvia , just needs time from a committer: typically over the weekend I run through all PRs in order of appearenace and merge them if ready. Thanks a for the patience.

nevi-me · 2020-11-27T20:23:08Z

@jorgecarleitao I've just gone through this, and don't have anything else to add. Can I merge it?

jorgecarleitao · 2020-11-27T20:27:56Z

@nevi-me , I went through it already. Thanks a lot!

velvia · 2020-11-27T21:56:52Z

Thanks everyone!

This fixes the CI. Was introduced in #8688 , I guess because the CI there ran before the merge. Closes #8791 from Dandandan/fix_clone Authored-by: Heres, Daniel <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

This PR implements the NULLIF() SQL function in DataFusion. It is implemented as a BuiltInScalarFunction, with a boolean kernel at the core which creates a new array with a modified null bitmap from the original array, based on the result of a boolean expression. When an input data item is equal to the right side in NULLIF(), then the item's nullity becomes set in the output array. Closes apache#8688 from velvia/evan/rust-datafusion-nullif-func Lead-authored-by: Evan Chan <[email protected]> Co-authored-by: Evan Chan <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

This fixes the CI. Was introduced in apache#8688 , I guess because the CI there ran before the merge. Closes apache#8791 from Dandandan/fix_clone Authored-by: Heres, Daniel <[email protected]> Signed-off-by: Neville Dipale <[email protected]>

github-actions bot added Component: Rust - DataFusion Component: Rust labels Nov 17, 2020

jorgecarleitao reviewed Nov 17, 2020

View reviewed changes

vertexclique reviewed Nov 17, 2020

View reviewed changes

jorgecarleitao reviewed Nov 18, 2020

View reviewed changes

rust/arrow/src/compute/kernels/boolean.rs Outdated Show resolved Hide resolved

rust/arrow/src/compute/kernels/boolean.rs Show resolved Hide resolved

rust/arrow/src/compute/kernels/boolean.rs Show resolved Hide resolved

velvia force-pushed the evan/rust-datafusion-nullif-func branch from d6b39ad to 2f40764 Compare November 18, 2020 22:21

vertexclique mentioned this pull request Nov 19, 2020

ARROW-10588: [Rust] Safe and parallel bit operations for Arrow #8664

Closed

Evan Chan and others added 15 commits November 23, 2020 13:47

[Rust][kernels] Add nullif boolean kernel

4e364ac

Add nullif_func as expression function

e0b4975

Add nullif as supported built-in function. SQL test doesn't pass yet.

1fa8567

Compilation fixes for latest master

ea2dc55

NULLIF: return type might be coerced; make return types consistent

bcb9822

Fix test

ee86ef6

Cargo fmt

431f32e

@jorgecarleitao's suggestion

b1048ad

Co-authored-by: Jorge Leitao <[email protected]>

Some CR feedback

f188050

Fix build issues, tests pass

09b6bc4

Fix null_counts in arrays created from nullif() kernel

ebc56e6

cargo fmt

8753cb1

Fix nullif kernel for arrays with nonzero offsets

9edc13c

Datafusion README: nullif support

308c341

Cargo fmt and import fixes for latest master

174db8e

velvia force-pushed the evan/rust-datafusion-nullif-func branch from 4cf478c to 174db8e Compare November 23, 2020 21:57

jhorstmann reviewed Nov 25, 2020

View reviewed changes

rust/arrow/src/compute/kernels/boolean.rs Outdated Show resolved Hide resolved

Clean up test and address right buffer bit slice

adc36bf

jorgecarleitao approved these changes Nov 26, 2020

View reviewed changes

nevi-me approved these changes Nov 27, 2020

View reviewed changes

nevi-me closed this in 2f77cc2 Nov 27, 2020

Dandandan mentioned this pull request Nov 28, 2020

ARROW-10756: [Rust][DataFusion] Fix reduntant clones #8791

Closed

asfimport mentioned this pull request Dec 24, 2020

[Rust][Datafusion] Implement nullif() function for DataFusion #26319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-10330: [Rust][DataFusion] Implement NULLIF() SQL function #8688

ARROW-10330: [Rust][DataFusion] Implement NULLIF() SQL function #8688

velvia commented Nov 17, 2020

velvia commented Nov 17, 2020

github-actions bot commented Nov 17, 2020

jorgecarleitao left a comment

vertexclique Nov 17, 2020

jorgecarleitao Nov 17, 2020 •

edited

Loading

vertexclique Nov 17, 2020

jorgecarleitao Nov 17, 2020 •

edited

Loading

vertexclique Nov 17, 2020

jorgecarleitao left a comment

velvia commented Nov 23, 2020

jorgecarleitao left a comment

velvia commented Nov 27, 2020

jorgecarleitao commented Nov 27, 2020

nevi-me commented Nov 27, 2020

jorgecarleitao commented Nov 27, 2020

velvia commented Nov 27, 2020

ARROW-10330: [Rust][DataFusion] Implement NULLIF() SQL function #8688

ARROW-10330: [Rust][DataFusion] Implement NULLIF() SQL function #8688

Conversation

velvia commented Nov 17, 2020

velvia commented Nov 17, 2020

github-actions bot commented Nov 17, 2020

jorgecarleitao left a comment

Choose a reason for hiding this comment

vertexclique Nov 17, 2020

Choose a reason for hiding this comment

jorgecarleitao Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

vertexclique Nov 17, 2020

Choose a reason for hiding this comment

jorgecarleitao Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

vertexclique Nov 17, 2020

Choose a reason for hiding this comment

jorgecarleitao left a comment

Choose a reason for hiding this comment

velvia commented Nov 23, 2020

jorgecarleitao left a comment

Choose a reason for hiding this comment

velvia commented Nov 27, 2020

jorgecarleitao commented Nov 27, 2020

nevi-me commented Nov 27, 2020

jorgecarleitao commented Nov 27, 2020

velvia commented Nov 27, 2020

jorgecarleitao Nov 17, 2020 •

edited

Loading

jorgecarleitao Nov 17, 2020 •

edited

Loading