[FEAT] add `list.value_counts()` #2902

andrewgazelka · 2024-09-24T04:24:40Z

still a few things to do particular with types being null or not... but at least should not crash now

TODO

add desc
make sure works for nested types "if no, or if we don't intend to support that usecase, we should assert the types allowed and add this to the doc"

daft/expressions/expressions.py

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

src/daft-core/src/array/fixed_size_list_array.rs

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

codspeed-hq · 2024-09-24T16:56:53Z

CodSpeed Performance Report

Merging #2902 will not alter performance

_{Comparing andrew/list-value_counts (fa23718) with main (cd59c73)}

Summary

✅ 17 untouched benchmarks

codecov · 2024-09-24T17:00:58Z

Codecov Report

Attention: Patch coverage is 77.87307% with 129 lines in your changes missing coverage. Please review.

Project coverage is 78.36%. Comparing base (46c5a7e) to head (fa23718).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
src/common/tracing/src/lib.rs	15.38%	22 Missing ⚠️
src/daft-core/src/array/fixed_size_list_array.rs	0.00%	20 Missing ⚠️
src/daft-core/src/array/mod.rs	39.28%	17 Missing ⚠️
src/daft-functions/src/list/value_counts.rs	61.90%	16 Missing ⚠️
src/daft-table/src/lib.rs	44.44%	10 Missing ⚠️
src/daft-dsl/src/functions/map/get.rs	52.94%	8 Missing ⚠️
src/daft-core/src/array/ops/from_arrow.rs	76.92%	6 Missing ⚠️
src/daft-core/src/series/ops/list.rs	57.14%	6 Missing ⚠️
src/daft-micropartition/src/micropartition.rs	88.88%	5 Missing ⚠️
src/daft-schema/src/schema.rs	54.54%	5 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2902      +/-   ##
==========================================
+ Coverage   76.34%   78.36%   +2.01%     
==========================================
  Files         597      603       +6     
  Lines       71388    71374      -14     
==========================================
+ Hits        54504    55934    +1430     
+ Misses      16884    15440    -1444

Flag	Coverage Δ
	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
daft/expressions/expressions.py	`93.67% <100.00%> (+0.29%)`	⬆️
src/daft-core/src/array/list_array.rs	`84.11% <100.00%> (+0.58%)`	⬆️
src/daft-core/src/array/ops/arrow2/comparison.rs	`98.70% <100.00%> (ø)`
src/daft-core/src/array/ops/cast.rs	`87.50% <100.00%> (+0.02%)`	⬆️
src/daft-core/src/array/ops/groups.rs	`98.18% <100.00%> (ø)`
src/daft-core/src/array/ops/map.rs	`87.50% <100.00%> (+14.77%)`	⬆️
src/daft-core/src/array/ops/sparse_tensor.rs	`100.00% <100.00%> (ø)`
src/daft-core/src/array/serdes.rs	`87.74% <100.00%> (+0.32%)`	⬆️
src/daft-core/src/array/struct_array.rs	`79.71% <ø> (ø)`
src/daft-core/src/datatypes/logical.rs	`74.07% <ø> (ø)`
... and 28 more

... and 78 files with indirect coverage changes

jaychia · 2024-09-26T20:42:47Z

daft/expressions/expressions.py

@@ -2922,6 +2922,17 @@ def join(self, delimiter: str | Expression) -> Expression:
        delimiter_expr = Expression._to_expression(delimiter)
        return Expression._from_pyexpr(native.list_join(self._expr, delimiter_expr._expr))

+    # todo: do we want type to be a Map expression? how should we do this?


There isnt a way to express the static type of an Expression in Python. It's just an Expression until it gets resolved against a dataframe's schema. At that point we can then use the Expression to generate a schema.

Let's rm this comment too before merge!

correct me if I am right but can't we return something like

Daft/daft/expressions/expressions.py

Line 3072 in 6e0944a

class ExpressionMapNamespace(ExpressionNamespace):

Ah great question -- not really because then we'd lose access to the other Expression functionality such as Expression.is_null().

There generally isn't a great solution here given the dynamic nature of how we're constructing these Expressions tbh. We can only definitively know what output type an Expression would produce once it is resolved against a schema. For example:

col("a") + col("b")

Should this expression produce a int32 type? The answer is: it depends. Because if col("a") and col("b") were string types, and we resolve this expression against that schema, then the output type will be a string.

I think we intended to remove this comment?

daft/expressions/expressions.py

src/arrow2/src/offset.rs

src/daft-core/src/array/list_array.rs

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

desmondcheongzx

Flushing what I have right now, seems you wanted to iterate a bit more on it.

Could we also update the PR description so we get a proper commit message?

desmondcheongzx · 2024-10-03T22:56:46Z

daft/expressions/expressions.py

@@ -2922,6 +2922,17 @@ def join(self, delimiter: str | Expression) -> Expression:
        delimiter_expr = Expression._to_expression(delimiter)
        return Expression._from_pyexpr(native.list_join(self._expr, delimiter_expr._expr))

+    # todo: do we want type to be a Map expression? how should we do this?


I think we intended to remove this comment?

src/arrow2/src/array/map/mod.rs

desmondcheongzx · 2024-10-03T23:02:10Z

src/arrow2/src/array/map/mod.rs

+        let outer_is_map = matches!(target, DataType::Map { .. });
+
+        if outer_is_map {
+            // we can do simple conversion


Nit: I think a more useful comment would be explaining why we can do this simple conversion, instead of stating that it's a simple conversion before doing a simple conversion.

src/arrow2/src/array/mod.rs

desmondcheongzx · 2024-10-03T23:09:25Z

src/arrow2/src/array/mod.rs

+                let from =                     format!("{:#?}", self.data_type());
+                let to = format!("{:#?}", data_type);
+
+


Uber nit: double newline intentional?

believe I changed this

desmondcheongzx · 2024-10-03T23:11:14Z

src/arrow2/src/array/mod.rs

+                let diff = diff
+                    .unified_diff();
+
+                panic!("{diff}");


Could we write a more descriptive error message to go with the diff of the types?

believe I changed this

desmondcheongzx · 2024-10-03T23:15:55Z

tests/integration/iceberg/test_table_load.py

@@ -24,7 +24,7 @@ def test_daft_iceberg_table_open(local_iceberg_tables):


 WORKING_SHOW_COLLECT = [
-    "test_all_types",
+    # "test_all_types",


We should leave a comment on why we're skipping this test, and create a corresponding github issue for the broken test

tests/table/map/test_map_get.py

desmondcheongzx · 2024-10-03T23:17:17Z

src/lib.rs

@@ -96,6 +96,7 @@ pub mod pylib {
    #[pymodule]
    fn daft(py: Python, m: &Bound<PyModule>) -> PyResult<()> {
        refresh_logger(py)?;
+


uber nit: stray whitespace from some previous change.

desmondcheongzx · 2024-10-03T23:17:35Z

tests/expressions/test_expressions.py

+        {"list_col": [["a", "b", "a", "c"], ["b", "b", "c"], ["a", "a", "a"], [], ["d", None, "d"]]}
+    )
+
+    # # Apply list_value_counts operation


uber nit: remove double comment?

desmondcheongzx

Discussing offline---what happens with lists of nested values?

Also, could we add this new expression to docs/source/api_docs/expressions.rst?

desmondcheongzx · 2024-10-04T00:56:48Z

src/daft-micropartition/src/micropartition.rs

-                            },
+                            }


nit: isn't it typical to leave the comma?

not according to our current rustfmt rules, no

desmondcheongzx · 2024-10-04T01:00:46Z

src/daft-core/src/array/mod.rs

-        if let Ok(arrow_dtype) = field.dtype.to_physical().to_arrow() {
-            if !arrow_dtype.eq(data.data_type()) {
+        if let Ok(expected_arrow_physical_type) = physical_field.dtype.to_arrow()
+        // For instance Int32 -> Int 32


What is this comment referring to?

think it was when I was debugging; fixed

e0ec7e0

desmondcheongzx · 2024-10-04T01:01:02Z

src/daft-core/src/array/mod.rs

+        if let Ok(expected_arrow_physical_type) = physical_field.dtype.to_arrow()
+        // For instance Int32 -> Int 32
+        {
+            let arrow_data_type = arrow_array.data_type(); // logical type


Nit: comment unnecessary?

src/daft-core/src/series/ops/concat.rs

src/daft-micropartition/src/ops/eval_expressions.rs

src/daft-schema/src/dtype.rs

desmondcheongzx · 2024-10-04T03:03:33Z

src/daft-schema/src/dtype.rs

@@ -638,7 +649,29 @@ impl From<&ArrowType> for DataType {
            ArrowType::FixedSizeList(field, size) => {
                Self::FixedSizeList(Box::new(field.as_ref().data_type().into()), *size)
            }
-            ArrowType::Map(field, ..) => Self::Map(Box::new(field.as_ref().data_type().into())),
+            ArrowType::Map(field, ..) => {
+                // todo: TryFrom in future? want in second pass maybe


Are we doing this TryForm? Fine if we don't

@jaychia and I want to do a TryFrom since it is fallible

desmondcheongzx · 2024-10-04T03:20:16Z

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

+        let flat_child = &*flat_child;
+
+        let is_equal = build_is_equal(
+            flat_child, flat_child, true, // todo: should nans be considered equal?


What was the verdict on this? NaNs should not be equal, but it would be interesting to allow the user to count the number of NaNs easily.

are NaNs considered invalid according to daft? If they are they should be ignored anyway but I agree we should make false probably. 🤔

if they aren't invalid we could get weird behavior. simulating in my head something like

NaN: 1
NaN: 1

...

etc

for value_counts

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

desmondcheongzx

Looks good, thanks for all the changes!

jaychia reviewed Sep 24, 2024

View reviewed changes

daft/expressions/expressions.py Outdated Show resolved Hide resolved

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

universalmind303 reviewed Sep 24, 2024

View reviewed changes

src/daft-core/src/array/fixed_size_list_array.rs Show resolved Hide resolved

universalmind303 reviewed Sep 24, 2024

View reviewed changes

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

andrewgazelka force-pushed the andrew/list-value_counts branch 4 times, most recently from b983578 to ba181af Compare September 25, 2024 19:10

andrewgazelka changed the title ~~value counts~~ [FEAT] add list.value_counts() Sep 25, 2024

github-actions bot added the enhancement New feature or request label Sep 25, 2024

andrewgazelka requested a review from jaychia September 25, 2024 21:02

andrewgazelka marked this pull request as ready for review September 25, 2024 21:13

andrewgazelka mentioned this pull request Sep 25, 2024

[CHORE] map type → key/value #2910

Closed

jaychia reviewed Sep 26, 2024

View reviewed changes

andrewgazelka force-pushed the andrew/list-value_counts branch from a4e1c7b to e063b04 Compare September 27, 2024 00:02

combine

4a6850a

andrewgazelka force-pushed the andrew/list-value_counts branch from 9aee66d to 4a6850a Compare October 1, 2024 20:16

andrewgazelka added 7 commits October 1, 2024 14:05

remove some unused deps

83748ac

fix bug kinda

f01c0ab

fixeroo yay that took forever

a6d33b2

fmt

33f1903

lint

0b3fb21

remove some logging

a372b74

remove test_all_types

45f885b

andrewgazelka requested review from jaychia and desmondcheongzx October 3, 2024 22:10

desmondcheongzx reviewed Oct 3, 2024

View reviewed changes

clean up code

dcc0a36

andrewgazelka force-pushed the andrew/list-value_counts branch from 78fa527 to dcc0a36 Compare October 3, 2024 23:54

desmondcheongzx reviewed Oct 4, 2024

View reviewed changes

andrewgazelka added 12 commits October 3, 2024 23:00

remove a tracing comment

141af62

remove tracing

f42524d

improve docs

321c351

add context

3b5b397

improvements / comments

e0ec7e0

remove comment

c86cedb

improve comment

cae76fd

add back include_mask

62894cf

remove todo

0139a6b

add nested

f3104c5

add degenerate test for list.value_counts

c3233d6

add docs

fa23718

desmondcheongzx approved these changes Oct 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] add `list.value_counts()` #2902

[FEAT] add `list.value_counts()` #2902

andrewgazelka commented Sep 24, 2024 •

edited

Loading

codspeed-hq bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

jaychia Sep 26, 2024

andrewgazelka Sep 27, 2024

jaychia Sep 27, 2024

desmondcheongzx Oct 3, 2024

desmondcheongzx left a comment

desmondcheongzx Oct 3, 2024

desmondcheongzx Oct 3, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 3, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 3, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 3, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 3, 2024

desmondcheongzx Oct 3, 2024

desmondcheongzx left a comment

desmondcheongzx Oct 4, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 4, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 4, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 4, 2024

andrewgazelka Oct 4, 2024

desmondcheongzx Oct 4, 2024

andrewgazelka Oct 4, 2024 •

edited

Loading

desmondcheongzx left a comment

		let from = format!("{:#?}", self.data_type());
		let to = format!("{:#?}", data_type);

[FEAT] add list.value_counts() #2902

Are you sure you want to change the base?

[FEAT] add list.value_counts() #2902

Conversation

andrewgazelka commented Sep 24, 2024 • edited Loading

codspeed-hq bot commented Sep 24, 2024 • edited Loading

Merging #2902 will not alter performance

Summary

codecov bot commented Sep 24, 2024 • 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

desmondcheongzx left a 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

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

Choose a reason for hiding this comment

desmondcheongzx left a 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

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

andrewgazelka Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

desmondcheongzx left a comment

Choose a reason for hiding this comment

[FEAT] add `list.value_counts()` #2902

[FEAT] add `list.value_counts()` #2902

andrewgazelka commented Sep 24, 2024 •

edited

Loading

codspeed-hq bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

andrewgazelka Oct 4, 2024 •

edited

Loading