[FEAT] Add a `list.contains` API #2773

raunakab · 2024-08-31T01:13:00Z

Overview

Added a list.contains API to check whether or not a column of type List or FixedSizeList contains a given value.

codspeed-hq · 2024-08-31T01:27:01Z

CodSpeed Performance Report

Merging #2773 will degrade performances by 27.25%

_{Comparing ronnie/list-contains (d63ce68) with main (d9d916b)}

Summary

❌ 1 regressions
✅ 16 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`ronnie/list-contains`	Change
❌	`test_iter_rows_first_row[100 Small Files]`	184.3 ms	253.3 ms	-27.25%

kevinzwang

An alternative way to write the kernel is to check equality between the flattened list and the values and then for each list range, determine if there are any true values in that equality array. The potential advantage of this is that you can use the vectorized arrow2 equality op. The potential disadvantage is that you can't early terminate if you find an equal value in a list.

Also, please add tests!

daft/expressions/expressions.py

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

kevinzwang · 2024-09-16T21:30:47Z

daft/expressions/expressions.py

+        Returns:
+            Expression: a List expression which are the lists which all contain at least one instance of the given value.
+        """
+        return Expression._from_pyexpr(native.list_contains(self._expr, value._expr))


You should convert value into an Expression using Expression._to_expression first before using it. That way it works with literals too.

How about self then?

Something like:

self_ = Expression(self)._to_expression() value = Expression(value)._to_expression() return Expression._from_pyexpr(native.list_contains(self_, value))

?

Self is already an expression so there's no need. It should look something like

value_expr = Expression.to_expression(value) return Expression._from_pyexpr(native.list_contains(self._expr, value_expr._expr))

Wait, I don't understand.

value is already of type Expression. self is of type ExpressionListNamespace. When I grab self._expr and value._expr, I am grabbing their internal PyExpr type, no? That is what the native.list_contains function expects.

Why should we perform the Expression._to_expression conversion first? Looking at the implementation, it seems to take in an object and return it untouched if it is of type Expression (and if not, return lit(object)).

kevinzwang · 2024-09-16T21:31:27Z

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

+
+    pub fn list_contains(&self, values: Series) -> DaftResult<BooleanArray> {
+        assert_eq!(self.len(), values.len(), "Expected two lists with the same length, instead got self.len() = {} and values.len() = {}", self.len(), values.len());
+        assert_eq!(self.child_data_type(), values.data_type(), "Expected values to be a column of type <T> and self to be column of type List<T>, but instead got values: {} and self: {}", values.data_type(), self.child_data_type());


You should consider casting values to the child dtype when possible so that it doesn't have to be exactly the same dtype.

kevinzwang · 2024-09-16T21:33:18Z

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

@@ -475,6 +476,31 @@ impl ListArray {
            self.validity().cloned(),
        ))
    }
+
+    pub fn list_contains(&self, values: Series) -> DaftResult<BooleanArray> {
+        assert_eq!(self.len(), values.len(), "Expected two lists with the same length, instead got self.len() = {} and values.len() = {}", self.len(), values.len());


values should be able to be either the same cardinality as self or a cardinality of 1 (for a literal). See the other list functions for some examples.

kevinzwang · 2024-09-16T21:42:38Z

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

+        assert_eq!(self.len(), values.len(), "Expected two lists with the same length, instead got self.len() = {} and values.len() = {}", self.len(), values.len());
+        assert_eq!(self.child_data_type(), values.data_type(), "Expected values to be a column of type <T> and self to be column of type List<T>, but instead got values: {} and self: {}", values.data_type(), self.child_data_type());
+        let founds = with_match_iterable_daft_types!(values.data_type(), |$T| {
+            let mut founds = vec![];


Consider using arrow2::array::BooleanArray::from_trusted_len_values_iter here instead if you can. That way you can construct an array directly from an iterator instead of creating a vec, which actually uses 8x more memory since boolean values are bytes in rust vectors but bits in the boolean array.

kevinzwang · 2024-09-16T21:43:31Z

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

@@ -701,6 +727,31 @@ impl FixedSizeListArray {
            self.validity().cloned(),
        ))
    }
+
+    pub fn list_contains(&self, values: Series) -> DaftResult<BooleanArray> {


same comments as ListArray::list_contains

kevinzwang · 2024-09-16T21:49:41Z

src/daft-functions/src/list/contains.rs

+                let value_field = value.to_field(schema)?;
+                match &data_field.dtype {
+                    DataType::List(inner_type) => {
+                        if inner_type.as_ref() == &value_field.dtype {


same comment about value type. if we can cast it to the inner type it should be okay

kevinzwang · 2024-09-16T21:52:37Z

src/daft-core/src/datatypes/matching.rs

@@ -151,6 +151,32 @@ macro_rules! with_match_comparable_daft_types {(
    }
 })}

+#[macro_export]
+macro_rules! with_match_iterable_daft_types {(


Not sure about the purpose of this macro. Shouldn't list contains work for any comparable type? You can use with_match_comparable_daft_types if so.

kevinzwang · 2024-09-16T22:00:36Z

src/daft-functions/src/list/contains.rs

+                let data_field = data.to_field(schema)?;
+                let value_field = value.to_field(schema)?;
+                match &data_field.dtype {
+                    DataType::List(inner_type) => {


Another check you should do with inner_type is to check if it is a type that you can check equality with the values.

Also, the dtype can be a FixedSizeList

samster25 · 2024-10-30T23:18:06Z

@raunakab @kevinzwang status on this PR?

jaychia · 2024-11-20T17:55:03Z

Hey folks, status? @raunakab

codecov · 2024-11-21T00:28:58Z

Codecov Report

Attention: Patch coverage is 2.46914% with 79 lines in your changes missing coverage. Please review.

Project coverage is 77.35%. Comparing base (5fee192) to head (d63ce68).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
src/daft-functions/src/list/contains.rs	0.00%	48 Missing ⚠️
src/daft-core/src/array/ops/list.rs	0.00%	16 Missing ⚠️
src/daft-core/src/series/ops/list.rs	0.00%	14 Missing ⚠️
daft/expressions/expressions.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2773      +/-   ##
==========================================
- Coverage   77.41%   77.35%   -0.07%     
==========================================
  Files         678      679       +1     
  Lines       83680    83332     -348     
==========================================
- Hits        64783    64460     -323     
+ Misses      18897    18872      -25

Files with missing lines	Coverage Δ
src/daft-functions/src/list/mod.rs	`100.00% <100.00%> (ø)`
daft/expressions/expressions.py	`93.26% <50.00%> (-0.12%)`	⬇️
src/daft-core/src/series/ops/list.rs	`58.55% <0.00%> (-5.95%)`	⬇️
src/daft-core/src/array/ops/list.rs	`92.39% <0.00%> (-2.14%)`	⬇️
src/daft-functions/src/list/contains.rs	`0.00% <0.00%> (ø)`

... and 10 files with indirect coverage changes

raunakab · 2024-12-03T16:32:17Z

This is a stale PR. Closing in favour of #3479.

Raunak Bhagat added 2 commits August 30, 2024 18:11

Create skeleton for list_contains API

b06102d

Implement list_contains for Series and Array

2d26b6b

github-actions bot added the enhancement New feature or request label Aug 31, 2024

raunakab marked this pull request as draft August 31, 2024 01:13

raunakab linked an issue Aug 31, 2024 that may be closed by this pull request

Add a .list.contains() expression #2749

Open

raunakab requested review from jaychia and Vince7778 August 31, 2024 01:13

Merge branch 'main' into ronnie/list-contains

430639e

raunakab marked this pull request as ready for review September 9, 2024 22:38

jaychia requested review from kevinzwang and removed request for jaychia and Vince7778 September 12, 2024 23:53

kevinzwang requested changes Sep 16, 2024

View reviewed changes

Raunak Bhagat added 2 commits November 20, 2024 16:06

Merge branch 'main' into ronnie/list-contains

a57824c

Merge branch 'main' into ronnie/list-contains

b4829bc

Raunak Bhagat added 2 commits November 20, 2024 16:59

Update type of arg in doc-comments

95bfbdb

Change message in assertion

d63ce68

raunakab closed this Dec 3, 2024

raunakab deleted the ronnie/list-contains branch December 3, 2024 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Add a `list.contains` API #2773

[FEAT] Add a `list.contains` API #2773

raunakab commented Aug 31, 2024

codspeed-hq bot commented Aug 31, 2024 •

edited

Loading

kevinzwang left a comment

kevinzwang Sep 16, 2024

raunakab Nov 21, 2024

raunakab Nov 21, 2024 •

edited

Loading

kevinzwang Nov 21, 2024

raunakab Nov 21, 2024

raunakab Nov 21, 2024 •

edited

Loading

kevinzwang Sep 16, 2024

kevinzwang Sep 16, 2024

kevinzwang Sep 16, 2024 •

edited

Loading

kevinzwang Sep 16, 2024

kevinzwang Sep 16, 2024

kevinzwang Sep 16, 2024

kevinzwang Sep 16, 2024

kevinzwang Sep 16, 2024

samster25 commented Oct 30, 2024

jaychia commented Nov 20, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

raunakab commented Dec 3, 2024

[FEAT] Add a list.contains API #2773

[FEAT] Add a list.contains API #2773

Conversation

raunakab commented Aug 31, 2024

Overview

codspeed-hq bot commented Aug 31, 2024 • edited Loading

Merging #2773 will degrade performances by 27.25%

Summary

Benchmarks breakdown

kevinzwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raunakab Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raunakab Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinzwang Sep 16, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samster25 commented Oct 30, 2024

jaychia commented Nov 20, 2024

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

raunakab commented Dec 3, 2024

[FEAT] Add a `list.contains` API #2773

[FEAT] Add a `list.contains` API #2773

codspeed-hq bot commented Aug 31, 2024 •

edited

Loading

raunakab Nov 21, 2024 •

edited

Loading

raunakab Nov 21, 2024 •

edited

Loading

kevinzwang Sep 16, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading