[FEAT] Add Expression.list.contains #1174

jaychia · 2023-07-19T01:02:05Z

Adds an Expression.list.contains function to check whether an element is contained within lists in another column

Related to: #993

codecov · 2023-07-19T01:10:56Z

Codecov Report

Merging #1174 (0ea30c7) into main (8fe5597) will increase coverage by 0.97%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 0ea30c7 differs from pull request most recent head 1b7ab89. Consider uploading reports for the commit 1b7ab89 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1174      +/-   ##
==========================================
+ Coverage   87.45%   88.43%   +0.97%     
==========================================
  Files          62       54       -8     
  Lines        5955     5558     -397     
==========================================
- Hits         5208     4915     -293     
+ Misses        747      643     -104

Files Changed	Coverage Δ
daft/expressions/expressions.py	`91.64% <100.00%> (-0.20%)`	⬇️

... and 34 files with indirect coverage changes

src/daft-core/src/array/ops/list.rs

jaychia · 2023-07-20T23:20:59Z

src/daft-core/src/array/ops/list.rs

@@ -35,6 +36,31 @@ fn join_arrow_list_of_utf8s(
        })
 }

+fn arrow_list_contains(
+    list_elements: impl Iterator<Item = Option<Box<dyn arrow2::array::Array>>>,
+    elements: impl Iterator<Item = Box<dyn arrow2::array::Array>>,


Seems like I'm using Box<dyn arrow2::array::Array> as a bootleg "AnyValue" here.

Technically, a single-element Series could be used as a bootleg AnyValue too 😛

samster25 · 2023-07-20T23:38:02Z

src/daft-core/src/array/ops/list.rs

@@ -35,6 +36,31 @@ fn join_arrow_list_of_utf8s(
        })
 }

+fn arrow_list_contains(
+    list_elements: impl Iterator<Item = Option<Box<dyn arrow2::array::Array>>>,
+    elements: impl Iterator<Item = Box<dyn arrow2::array::Array>>,


samster25 · 2023-07-20T23:38:14Z

src/daft-core/src/array/ops/list.rs

+    list_elements: impl Iterator<Item = Option<Box<dyn arrow2::array::Array>>>,
+    elements: impl Iterator<Item = Box<dyn arrow2::array::Array>>,
+) -> DaftResult<arrow2::array::BooleanArray> {
+    let mut contains_elements: Vec<Option<bool>> = Vec::new();


no need for intermediate memory here

samster25 · 2023-07-20T23:38:41Z

src/daft-core/src/array/ops/list.rs

+    elements: impl Iterator<Item = Box<dyn arrow2::array::Array>>,
+) -> DaftResult<arrow2::array::BooleanArray> {
+    let mut contains_elements: Vec<Option<bool>> = Vec::new();
+    for (arr, element) in list_elements.zip(elements) {


can run this as a map and give an iterator then then create BooleanArray with from_iter: https://github.com/jorgecarleitao/arrow2/blob/main/src/array/boolean/from.rs#L12

Oh yes, I was going to do that and forgot... Thanks!

Hmm actually, I'm unable to do this because the code that executes in the Iterator can potentially return a DaftError (we run a .equals in that code).

The iterator that returns thus can't be piped into BooleanArray. The only way I could find to make it work still requires intermediate memory:

let results: Result<Vec<_>> = list_elements.zip(elements).map(...some code that throws errors).collect(); Ok(BooleanArray::from(results?))

Is there a better way? I guess we could just panic in there instead of throwing an error.

samster25 · 2023-07-20T23:41:47Z

src/daft-core/src/array/ops/list.rs

+                Box::new(std::iter::repeat(elements.to_arrow()))
+            } else {
+                assert!(elements.len() == self.len());
+                Box::new((0..self.len()).map(|i| elements.inner.to_arrow().sliced(i, 1)))


I think you actually want as_arrow: https://github.com/Eventual-Inc/Daft/blob/main/src/daft-core/src/array/ops/as_arrow.rs#L21. to_arrow is for exporting through ffi

Also once you call as_arrow you should be able to call .into_iter() on the concrete type https://github.com/jorgecarleitao/arrow2/blob/main/src/array/fixed_size_list/iterator.rs

I can't do as_arrow because it is only defined over DataArray, and I can't get the DataArray for elements because I don't know the concrete type of elements.inner without doing daft_match... statement on the dtype.

Is there a better workaround here?

I guess I could add an Series::as_arrow(), but that feels like it would be pretty confusing vs Series::to_arrow()?

Edit: Maybe we could do:

Add Series::as_arrow() which delegates to self.inner.downcast().as_arrow() by matching over the Series' dtype

Rename Series::to_arrow() to something like Series::export_arrow_for_ffi()

I would probably go that route since that would be correct. you can do a with_match_physical after converting it to the physical array. and Since you know that self.child and elements are the same dtype, it should be just be 1 match.

I would probably go that route

Are you referring to my suggestion: " I can't get the DataArray for elements because I don't know the concrete type of elements.inner without doing daft_match... statement on the dtype."?

I just tried your suggestion, but am running into more problems

let elements = elements.as_physical()?; with_match_physical_daft_types!(elements.data_type(), |$T| { let elements = elements.downcast::<$T>()?; elements.as_arrow() // <- Failing here because of ExtensionType, which seems to be a PhysicalType but I guess it doesn't map directly to an arrow2 concrete type array? })

In general our codebase feels really hard to work with when we have more than one level of types and the nested types are unknown... Super gnarly.

samster25 · 2023-07-20T23:43:42Z

src/daft-core/src/array/ops/list.rs

+                Box::new(std::iter::repeat(elements.to_arrow()))
+            } else {
+                assert!(elements.len() == self.len());
+                Box::new((0..self.len()).map(|i| elements.inner.to_arrow().sliced(i, 1)))


same as above: use as_arrow and user the into_iter method

github-actions bot added the enhancement New feature or request label Jul 19, 2023

jaychia mentioned this pull request Jul 19, 2023

Add an "in" filter predicate #993

Closed

github-actions bot added the documentation Improvements or additions to documentation label Jul 20, 2023

jaychia commented Jul 20, 2023

View reviewed changes

src/daft-core/src/array/ops/list.rs Show resolved Hide resolved

jaychia requested a review from samster25 July 20, 2023 23:18

jaychia commented Jul 20, 2023

View reviewed changes

samster25 reviewed Jul 20, 2023

View reviewed changes

Jay Chia added 8 commits August 9, 2023 12:30

[FEAT] Add Expression.list.contains

47b538c

Add unit tests

9ee445b

Switch from arrow2 compute to our own kernel

9d111ea

Object detection tutorial

17960d1

Refactor to share code

b4dd6b0

Enable fixedsizelist

ec80487

Remove dependency to compute::contains

4f52d84

rm tutorial notebook

1b7ab89

jaychia force-pushed the jay/list-contains branch from 0ea30c7 to 1b7ab89 Compare August 9, 2023 19:30

jaychia closed this Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Add Expression.list.contains #1174

[FEAT] Add Expression.list.contains #1174

jaychia commented Jul 19, 2023

codecov bot commented Jul 19, 2023 •

edited

Loading

jaychia Jul 20, 2023

samster25 Jul 20, 2023

samster25 Jul 20, 2023

samster25 Jul 20, 2023

samster25 Jul 20, 2023

jaychia Jul 20, 2023

jaychia Jul 21, 2023 •

edited

Loading

samster25 Jul 20, 2023

samster25 Jul 20, 2023

jaychia Jul 21, 2023 •

edited

Loading

jaychia Jul 21, 2023 •

edited

Loading

samster25 Jul 21, 2023

jaychia Jul 22, 2023

samster25 Jul 20, 2023

[FEAT] Add Expression.list.contains #1174

[FEAT] Add Expression.list.contains #1174

Conversation

jaychia commented Jul 19, 2023

codecov bot commented Jul 19, 2023 • edited Loading

Codecov Report

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

jaychia Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaychia Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

jaychia Jul 21, 2023 • edited Loading

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

codecov bot commented Jul 19, 2023 •

edited

Loading

jaychia Jul 21, 2023 •

edited

Loading

jaychia Jul 21, 2023 •

edited

Loading

jaychia Jul 21, 2023 •

edited

Loading