[FEAT] Support intersect as a DataFrame API #3134

advancedxy · 2024-10-28T07:22:55Z

This commit leverages null safe equal support in joins(see #3069 and #3161) to support intersect API.

Partially fixes #3122.

advancedxy · 2024-10-28T07:33:17Z

src/daft-dsl/src/lit.rs

                StructArray::new(struct_field, values, None).into_series()
            }
        }
    }
-
+    pub fn to_series(&self) -> Series {


Previous to_series doesn't work with Struct, as struct has its own field names but to_series always generate field with "literal".

advancedxy · 2024-10-28T07:33:54Z

src/daft-dsl/src/lit.rs

+            #[cfg(feature = "python")]
+            DataType::Python => {
+                use pyo3::prelude::*;
+                Self::Python(PyObjectWrapper(Python::with_gil(|py| py.None())))


Not sure whether None is the right choice for Python type.

codspeed-hq · 2024-10-28T07:35:57Z

CodSpeed Performance Report

Merging #3134 will not alter performance

_{Comparing advancedxy:intersect_operation (dcddfb5) with main (baca61e)}

Summary

✅ 17 untouched benchmarks

advancedxy · 2024-10-28T07:44:05Z

@kevinzwang @universalmind303 appreciated if you can take a look at this.

daft/dataframe/dataframe.py

advancedxy · 2024-10-29T11:42:49Z

daft/expressions/expressions.py

@@ -133,6 +134,39 @@ def lit(value: object) -> Expression:
    return Expression._from_pyexpr(lit_value)


+def zero_lit(dt: DataType) -> Expression:


It might be useful to expose this to Python side as well.

I can remove this if it's not desired.

src/daft-plan/src/logical_ops/set_operations.rs

kevinzwang

Hi @advancedxy, thank you for working on this! Really appreciate the work that you've done.

I don't have specific comments about the code yet, but from a cursory look at the PR, I don't see the need for the zero_lit expression. If you take a look at our join functions, we use arrow2's build_multi_array_is_equal to construct an equality check, and that function takes an argument nulls_equal.

Daft/src/daft-table/src/ops/joins/hash_join.rs

Lines 55 to 60 in 975c09e

    
           let is_equal = build_multi_array_is_equal( 
        
               lkeys.columns.as_slice(), 
        
               rkeys.columns.as_slice(), 
        
               false, 
        
               false, 
        
           )?;

I believe if we propagate a variable to set that, a null-safe join would automatically work, since the hashes used by the probe are already properly constructed for nulls as well.

Could you give that a try?

advancedxy · 2024-10-31T01:34:49Z

I believe if we propagate a variable to set that, a null-safe join would automatically work, since the hashes used by the probe are already properly constructed for nulls as well.

Hey @kevinzwang kevin, that was exactly my first thought as well. When I created the original issue, I think we can add null equal safe joins first and then leverage that to support the intersect operation. However, passing the parameter from the python side all the way down to the Rust's physical join plan, it seems it might touch a lot of code and I referenced other(a.k.a Spark) query engine's implementation and noticed that null safe equality could be effective rewrote as nvl + is_null, thus this PR. It might have other benefits to open more optimization opportunities when the null safe equality is rewritten. However, that's not the case for Arrow/Daft right now. I'm ok to not introduce that expression then.

Could you give that a try?

Of course, and taking a step back from here, I think we can add null safe equal in joins in the Rust side first, then leverage that to support this PR's intersection operator and then finish Python side's API. How does that sound to you?

kevinzwang · 2024-10-31T18:58:45Z

Of course, and taking a step back from here, I think we can add null safe equal in joins in the Rust side first, then leverage that to support this PR's intersection operator and then finish Python side's API. How does that sound to you?

Yep, that sounds like a good plan. Thank you again for taking this on!

advancedxy · 2024-11-04T03:24:12Z

Hey @kevinzwang @universalmind303 PTAL at this after #3161 is merged, thanks.

advancedxy · 2024-11-06T03:13:28Z

The CI failure seems unrelated. Close and re-open to trigger a new CI run.

advancedxy · 2024-11-07T06:23:56Z

Since #3161 is merged, I think this is ready for review.

universalmind303 · 2024-11-08T04:17:56Z

src/daft-logical-plan/src/builder.rs

+    pub fn intersect(&self, other: &Self, is_all: bool) -> DaftResult<Self> {
+        let logical_plan: LogicalPlan =
+            ops::Intersect::try_new(self.plan.clone(), other.plan.clone(), is_all)?
+                .to_optimized_join()?;
+        Ok(self.with_new_plan(logical_plan))
+    }


instead of immediately converting it to a join, I think we should instead defer this until translate stage.

If we want to have a concept of a logical Intersect as discussed here, then I don't think we should immediately convert it to a logical join, but only convert it once to a physical join during the logical -> physical translation.

I think we should instead defer this until translate stage.

That's definitely an option. And DuckDB does that indeed: https://github.com/duckdb/duckdb/blob/a2dce8b1c9fa6039c82e9a32bfcc4c49b03ca871/src/execution/physical_plan/plan_set_operation.cpp#L53. The problem is like you stated in #3241 (comment), it will make the optimization rules more complex: a lot of rules should be amend to be set-operations aware.

On the other side, Spark doesn't defer this to translate stage, the intersect/except operators are optimized during the optimization phase, which makes the optimization phase simpler(at least for set-operation handling).

From my perspective, I agree with you that we should convert set operations to join early to avoid complexing optimization phase currently. I just want to decouple the logical into a separate func/struct and make sure it's extensible for long term plan. I can avoid define these operations if that doesn't sound right to you. WDYT?

ok, Lets go with what's in here now. We can always revisit it needed 😅

stale

github-actions bot added the enhancement New feature or request label Oct 28, 2024

advancedxy commented Oct 28, 2024

View reviewed changes

universalmind303 reviewed Oct 28, 2024

View reviewed changes

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

advancedxy force-pushed the intersect_operation branch from 51da893 to f0ed52b Compare October 29, 2024 11:40

advancedxy commented Oct 29, 2024

View reviewed changes

src/daft-plan/src/logical_ops/set_operations.rs Outdated Show resolved Hide resolved

kevinzwang self-requested a review October 29, 2024 17:04

kevinzwang previously requested changes Oct 30, 2024

View reviewed changes

advancedxy force-pushed the intersect_operation branch from 692704e to 31af0e7 Compare November 4, 2024 03:20

advancedxy force-pushed the intersect_operation branch from 31af0e7 to 8b9fda5 Compare November 4, 2024 03:28

advancedxy closed this Nov 4, 2024

advancedxy reopened this Nov 4, 2024

advancedxy force-pushed the intersect_operation branch from 8b9fda5 to 82e545d Compare November 6, 2024 02:23

advancedxy closed this Nov 6, 2024

advancedxy reopened this Nov 6, 2024

advancedxy mentioned this pull request Nov 6, 2024

[FEAT] Support null equal safe join in SQL #3166

Merged

advancedxy added 2 commits November 7, 2024 10:53

[FEAT] Support intersect as a DataFrame API

6de169c

[CHORE] Make Intersect a LogicalPlan node

dcddfb5

advancedxy force-pushed the intersect_operation branch from 82e545d to dcddfb5 Compare November 7, 2024 02:53

advancedxy closed this Nov 7, 2024

advancedxy reopened this Nov 7, 2024

universalmind303 requested a review from kevinzwang November 8, 2024 04:09

universalmind303 reviewed Nov 8, 2024

View reviewed changes

universalmind303 approved these changes Nov 12, 2024

View reviewed changes

universalmind303 merged commit a547bd3 into Eventual-Inc:main Nov 12, 2024
73 of 75 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] Support intersect as a DataFrame API #3134

[FEAT] Support intersect as a DataFrame API #3134

advancedxy commented Oct 28, 2024 •

edited

Loading

advancedxy Oct 28, 2024

advancedxy Oct 28, 2024

codspeed-hq bot commented Oct 28, 2024 •

edited

Loading

advancedxy commented Oct 28, 2024

advancedxy Oct 29, 2024 •

edited

Loading

kevinzwang left a comment

advancedxy commented Oct 31, 2024

kevinzwang commented Oct 31, 2024

advancedxy commented Nov 4, 2024

advancedxy commented Nov 6, 2024

advancedxy commented Nov 7, 2024

universalmind303 Nov 8, 2024 •

edited

Loading

advancedxy Nov 12, 2024

universalmind303 Nov 12, 2024

		@@ -133,6 +134,39 @@ def lit(value: object) -> Expression:
		return Expression._from_pyexpr(lit_value)


		def zero_lit(dt: DataType) -> Expression:

	let is_equal = build_multi_array_is_equal(
	lkeys.columns.as_slice(),
	rkeys.columns.as_slice(),
	false,
	false,
	)?;

[FEAT] Support intersect as a DataFrame API #3134

[FEAT] Support intersect as a DataFrame API #3134

Conversation

advancedxy commented Oct 28, 2024 • edited Loading

advancedxy Oct 28, 2024

Choose a reason for hiding this comment

advancedxy Oct 28, 2024

Choose a reason for hiding this comment

codspeed-hq bot commented Oct 28, 2024 • edited Loading

CodSpeed Performance Report

Merging #3134 will not alter performance

Summary

advancedxy commented Oct 28, 2024

advancedxy Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

kevinzwang left a comment

Choose a reason for hiding this comment

advancedxy commented Oct 31, 2024

kevinzwang commented Oct 31, 2024

advancedxy commented Nov 4, 2024

advancedxy commented Nov 6, 2024

advancedxy commented Nov 7, 2024

universalmind303 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

advancedxy Nov 12, 2024

Choose a reason for hiding this comment

universalmind303 Nov 12, 2024

Choose a reason for hiding this comment

advancedxy commented Oct 28, 2024 •

edited

Loading

codspeed-hq bot commented Oct 28, 2024 •

edited

Loading

advancedxy Oct 29, 2024 •

edited

Loading

universalmind303 Nov 8, 2024 •

edited

Loading