[FEAT] connect: `df.join` #3354

andrewgazelka · 2024-11-20T09:12:28Z

fix

E           pyspark.errors.exceptions.connect.SparkConnectGrpcException: <_MultiThreadedRendezvous of RPC that terminated with:
E               status = StatusCode.INTERNAL
E               details = "Error in Daft server: Not Yet Implemented: Joins without join conditions (cross join) are not supported yet
E           
E           Location:
E               src/daft-connect/src/op/execute/root.rs:37:39"
E               debug_error_string = "UNKNOWN:Error received from peer ipv4:127.0.0.1:55241 {created_time:"2024-11-21T09:18:54.065079-08:00", grpc_status:13, grpc_message:"Error in Daft server: Not Yet Implemented: Joins without join conditions (cross join) are not supported yet\n\nLocation:\n    src/daft-connect/src/op/execute/root.rs:37:39"}"
E           >

andrewgazelka · 2024-11-20T09:12:54Z

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

[FEAT] connect: df.join #3354 👈 (View in Graphite)
[FEAT] connect: with_columns_renamed #3386
[FEAT] connect: add df.{intersection,union} #3373 : 5 other dependent PRs (#3375 , #3377 , #3379 and 2 others)
[FEAT] connect: df.withColumn #3359
[FEAT] connect: add groupBy #3351
[FEAT] connect: add binary operators #3350 : 3 other dependent PRs (#3346 , #3376 , #3387 )
[FEAT] connect: support basic column operations #3362 : 1 other dependent PR (#3363 )
[FEAT] connect: to_daft_* use ref instead of value #3355 : 2 other dependent PRs (#3357 , #3360 )
[FEAT] connect: add alias support #3342 : 6 other dependent PRs (#3344 , #3345 , #3348 and 3 others)
[FEAT] consolidate Spark session fixture into conftest.py #3341 : 3 other dependent PRs (#3309 , #3326 , #3343 )
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

graphite-app · 2024-11-21T05:20:11Z

Graphite Automations

"Notify author when CI fails" took an action on this PR • (11/21/24)

1 teammate was notified to this PR based on Andrew Gazelka's automation.

codspeed-hq · 2024-11-21T05:21:11Z

CodSpeed Performance Report

Merging #3354 will degrade performances by 54.24%

_{Comparing andrew/connect-join (2c4cb41) with main (cbe9d3b)}

Summary

❌ 2 regressions
✅ 15 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	`main`	`andrew/connect-join`	Change
❌	`test_iter_rows_first_row[100 Small Files]`	219.4 ms	262.8 ms	-16.51%
❌	`test_show[100 Small Files]`	15 ms	32.7 ms	-54.24%

codecov · 2024-11-21T05:47:31Z

Codecov Report

Attention: Patch coverage is 83.01887% with 9 lines in your changes missing coverage. Please review.

Project coverage is 77.35%. Comparing base (3394a66) to head (a56ba92).

Files with missing lines	Patch %	Lines
.../daft-connect/src/translation/logical_plan/join.rs	82.69%	9 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3354   +/-   ##
=======================================
  Coverage   77.35%   77.35%           
=======================================
  Files         685      686    +1     
  Lines       83631    83682   +51     
=======================================
+ Hits        64695    64735   +40     
- Misses      18936    18947   +11

Files with missing lines	Coverage Δ
src/daft-connect/src/translation/logical_plan.rs	`64.28% <100.00%> (+2.74%)`	⬆️
.../daft-connect/src/translation/logical_plan/join.rs	`82.69% <82.69%> (ø)`

... and 3 files with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

src/daft-connect/src/translation/logical_plan/join.rs

universalmind303 · 2024-11-21T15:36:04Z

src/daft-connect/src/translation/logical_plan/join.rs

+        None,
+        None,
+        None,
+        false, // todo(correctness): we want join keys or not


I'm not sure what spark expects here. Usually SQL will retain the join keys, and most dataframes will not. I'd try to perform a simple join with spark and see what happens.

universalmind303 · 2024-11-21T15:37:41Z

src/daft-connect/src/translation/logical_plan/join.rs

+    if let Some(join_condition) = join_condition {
+        bail!("Join conditions are not yet supported; use using_columns (join keys) instead; got {join_condition:?}");
+    }


we should definitely support both USING and regular joins. They are mutually exclusive though, so we'll need to error if both are provided

universalmind303 · 2024-11-21T15:41:28Z

src/daft-connect/src/translation/logical_plan/join.rs

+        bail!("Right side of join is required");
+    };
+
+    if let Some(join_condition) = join_condition {


so we'll need some complicated logic to split the join condition into left and right halves. Check out the SQL frontend. We're doing something similar to split the join condition

universalmind303 · 2024-11-21T15:42:47Z

tests/connect/test_join.py

+def test_join(spark_session):
+    # Create two DataFrames with overlapping IDs
+    df1 = spark_session.range(5)
+    df2 = spark_session.range(3, 7)
+
+    # Perform inner join on 'id' column
+    joined_df = df1.join(df2, "id", "inner")
+
+    # Verify join results
+    # assert joined_df.count() == 2, "Inner join should only return matching rows"
+
+    # Convert to pandas to verify exact results
+    joined_pandas = joined_df.toPandas()
+    assert set(joined_pandas["id"].tolist()) == {3, 4}, "Inner join should only contain IDs 3 and 4"
+
+    # Test left outer join
+    left_joined_pandas = df1.join(df2, "id", "left").toPandas()
+    assert set(left_joined_pandas["id"].tolist()) == {0, 1, 2, 3, 4}, "Left join should keep all rows from left DataFrame"
+
+    # Test right outer join 
+    right_joined_pandas = df1.join(df2, "id", "right").toPandas()
+    assert set(right_joined_pandas["id"].tolist()) == {3, 4, 5, 6}, "Right join should keep all rows from right DataFrame"


[Re: lines +6 to +28]

we're going to need a lot more tests for joins. Check out the other join tests, both for dataframe and sql.

See this comment inline on Graphite.

universalmind303 · 2024-11-21T15:45:06Z

src/daft-connect/src/translation/logical_plan/join.rs

+            bail!("Join type must be specified; got Unspecified")
+        }
+        JoinType::Inner => Ok(daft_core::join::JoinType::Inner),
+        JoinType::FullOuter => {


FullOuter is just the same as Outer

universalmind303 · 2024-11-21T15:45:24Z

src/daft-connect/src/translation/logical_plan/join.rs

+        }
+        JoinType::LeftOuter => Ok(daft_core::join::JoinType::Left), // todo(correctness): is this correct?
+        JoinType::RightOuter => Ok(daft_core::join::JoinType::Right),
+        JoinType::LeftAnti => Ok(daft_core::join::JoinType::Anti), // todo(correctness): is this correct?


remove comment. This is correct

universalmind303 · 2024-11-21T15:45:34Z

src/daft-connect/src/translation/logical_plan/join.rs

+        JoinType::FullOuter => {
+            bail!("Full outer joins not yet supported") // todo(completeness): add support for full outer joins if it is not already implemented
+        }
+        JoinType::LeftOuter => Ok(daft_core::join::JoinType::Left), // todo(correctness): is this correct?


remove comment. this is correct

universalmind303 · 2024-11-21T15:46:30Z

src/daft-connect/src/translation/logical_plan/join.rs

+        None,
+        None,


does spark suffix/prefix the duplicate join keys at all? We should verify and match what they do here.

graphite-app · 2024-11-21T17:37:23Z

src/daft-connect/src/translation/logical_plan/join.rs

+
+impl From<daft_logical_plan::JoinType> for JoinTypeInfo {
+    fn from(join_type: daft_logical_plan::JoinType) -> Self {
+        JoinTypeInfo::Regular(join_type)


Replace JoinTypeInfo::Regular(join_type) with Self::Regular(join_type) to follow Rust style guidelines for impl From blocks

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-11-21T17:37:28Z

src/daft-connect/src/translation/logical_plan/join.rs

+    let result = match join_type {
+        JoinTypeInfo::Cross => {
+            left.cross_join(&right, None, None)? // todo(correctness): is this correct?


Cross joins are not supported in Daft yet. This should be replaced with a bail!() statement that indicates cross joins are not supported.

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

universalmind303 · 2024-11-22T22:10:54Z

tests/connect/test_join.py

+
+
+
+def test_cross_join(spark_session):


we don't support cross joins yet

andrewgazelka marked this pull request as ready for review November 20, 2024 09:12

andrewgazelka force-pushed the andrew/connect-join branch from 4ee6017 to f40788c Compare November 20, 2024 09:25

andrewgazelka changed the base branch from andrew/connect-alias to andrew/connect-refactor-to-ref November 20, 2024 09:25

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from 863ac08 to c999ae9 Compare November 20, 2024 18:24

andrewgazelka force-pushed the andrew/connect-join branch from f40788c to d706ff8 Compare November 20, 2024 18:25

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from c999ae9 to 394c381 Compare November 20, 2024 18:32

andrewgazelka force-pushed the andrew/connect-join branch from d706ff8 to 264817c Compare November 20, 2024 18:33

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from 394c381 to 3fb2d4f Compare November 20, 2024 18:43

andrewgazelka force-pushed the andrew/connect-join branch from 264817c to 252d2b4 Compare November 20, 2024 18:43

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from 3fb2d4f to 4f49d30 Compare November 20, 2024 18:48

andrewgazelka force-pushed the andrew/connect-join branch from 252d2b4 to ee02aa1 Compare November 20, 2024 18:49

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from 4f49d30 to e95abbd Compare November 20, 2024 19:32

andrewgazelka force-pushed the andrew/connect-join branch from ee02aa1 to 22f9234 Compare November 20, 2024 19:32

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from e95abbd to 0e54699 Compare November 20, 2024 22:12

andrewgazelka force-pushed the andrew/connect-join branch from 22f9234 to 9375511 Compare November 20, 2024 23:25

andrewgazelka force-pushed the andrew/connect-refactor-to-ref branch from 0e54699 to cdcd749 Compare November 20, 2024 23:28

andrewgazelka force-pushed the andrew/connect-join branch from 9375511 to 7227e42 Compare November 20, 2024 23:29

andrewgazelka mentioned this pull request Nov 21, 2024

[FEAT] connect: add df.{intersection,union} #3373

Draft

andrewgazelka force-pushed the andrew/connect-join branch from 7350795 to c52c7a0 Compare November 21, 2024 04:19

This was referenced Nov 21, 2024

[TEST] connect: df.orderBy #3381

Draft

[FEAT] connect: add repartition support #3382

Draft

[FEAT] connect: support sample #3384

Draft

andrewgazelka changed the base branch from andrew/connect-refactor-to-ref to graphite-base/3354 November 21, 2024 05:11

andrewgazelka force-pushed the graphite-base/3354 branch from db3b512 to 3394a66 Compare November 21, 2024 05:11

andrewgazelka force-pushed the andrew/connect-join branch from c52c7a0 to 17f65ec Compare November 21, 2024 05:11

andrewgazelka changed the base branch from graphite-base/3354 to main November 21, 2024 05:12

andrewgazelka force-pushed the andrew/connect-join branch from 17f65ec to a56ba92 Compare November 21, 2024 05:12

github-actions bot added the enhancement New feature or request label Nov 21, 2024

andrewgazelka mentioned this pull request Nov 21, 2024

[FEAT] connect: with_columns_renamed #3386

Draft

andrewgazelka mentioned this pull request Nov 21, 2024

[FEAT] connect: add more (internally P2) column operations (WIP) #3387

Draft

graphite-app bot reviewed Nov 21, 2024

View reviewed changes

src/daft-connect/src/translation/logical_plan/join.rs Outdated Show resolved Hide resolved

universalmind303 requested changes Nov 21, 2024

View reviewed changes

andrewgazelka force-pushed the andrew/connect-join branch 3 times, most recently from dd5e05c to 2c4cb41 Compare November 21, 2024 17:19

andrewgazelka changed the base branch from main to andrew/connect-with-columns-renamed November 21, 2024 17:19

graphite-app bot reviewed Nov 21, 2024

View reviewed changes

andrewgazelka force-pushed the andrew/connect-with-columns-renamed branch from bb9a07f to e5c3a8d Compare November 21, 2024 18:44

andrewgazelka force-pushed the andrew/connect-join branch from 2c4cb41 to 02a0e34 Compare November 21, 2024 18:44

universalmind303 reviewed Nov 22, 2024

View reviewed changes

tests/connect/test_join.py

def test_cross_join(spark_session):

Copy link

Contributor

universalmind303 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't support cross joins yet

[FEAT] connect: df.join

1b0de2e

andrewgazelka force-pushed the andrew/connect-with-columns-renamed branch from e5c3a8d to af8b7ca Compare November 27, 2024 07:56

andrewgazelka force-pushed the andrew/connect-join branch from 02a0e34 to 1b0de2e Compare November 27, 2024 07:56

andrewgazelka marked this pull request as draft November 27, 2024 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEAT] connect: `df.join` #3354

[FEAT] connect: `df.join` #3354

andrewgazelka commented Nov 20, 2024 •

edited

Loading

andrewgazelka commented Nov 20, 2024 •

edited

Loading

graphite-app bot commented Nov 21, 2024

codspeed-hq bot commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

universalmind303 Nov 21, 2024

graphite-app bot Nov 21, 2024

graphite-app bot Nov 21, 2024

universalmind303 Nov 22, 2024

[FEAT] connect: df.join #3354

Are you sure you want to change the base?

[FEAT] connect: df.join #3354

Conversation

andrewgazelka commented Nov 20, 2024 • edited Loading

andrewgazelka commented Nov 20, 2024 • edited Loading

graphite-app bot commented Nov 21, 2024

Graphite Automations

codspeed-hq bot commented Nov 21, 2024 • edited Loading

Merging #3354 will degrade performances by 54.24%

Summary

Benchmarks breakdown

codecov bot commented Nov 21, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graphite-app bot Nov 21, 2024

Choose a reason for hiding this comment

graphite-app bot Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[FEAT] connect: `df.join` #3354

[FEAT] connect: `df.join` #3354

andrewgazelka commented Nov 20, 2024 •

edited

Loading

andrewgazelka commented Nov 20, 2024 •

edited

Loading

codspeed-hq bot commented Nov 21, 2024 •

edited

Loading