fix(join): joining on different types #3716

kevinzwang · 2025-01-21T19:53:16Z

This PR fixes a few things and also has some QOL changes.
Fixes:

Joining on null-type join keys
Joining on empty table (resolves Joins on empty tables give exceptions #3071) which turned out to be the above issue
Joining on join keys with different types
Combined column typing (right and outer joins should not just give left column types)

QOL:

Combine all the column renaming parameters into a JoinOptions type. Reduces the parameters for a bunch of functions and also uses the builder pattern
Rename keep_join_keys field to merge_matching_join_keys to make behavior more clear

codspeed-hq · 2025-01-21T23:29:34Z

CodSpeed Performance Report

Merging #3716 will degrade performances by 44.76%

_{Comparing kevin/fix-join-diff-types (449742e) with main (de5acf5)}

Summary

❌ 1 regressions
✅ 26 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`test_iter_rows_first_row[100 Small Files]`	122.3 ms	221.4 ms	-44.76%

codecov · 2025-01-21T23:54:21Z

Codecov Report

Attention: Patch coverage is 87.73006% with 40 lines in your changes missing coverage. Please review.

Project coverage is 77.56%. Comparing base (fec399e) to head (449742e).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
src/daft-logical-plan/src/ops/join.rs	56.71%	29 Missing ⚠️
src/daft-dsl/src/expr/mod.rs	84.00%	4 Missing ⚠️
src/daft-local-execution/src/pipeline.rs	80.00%	3 Missing ⚠️
src/daft-logical-plan/src/treenode.rs	50.00%	2 Missing ⚠️
src/daft-dsl/src/join.rs	98.63%	1 Missing ⚠️
...local-execution/src/sinks/outer_hash_join_probe.rs	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3716      +/-   ##
==========================================
- Coverage   77.58%   77.56%   -0.02%     
==========================================
  Files         729      728       -1     
  Lines       92010    92054      +44     
==========================================
+ Hits        71389    71406      +17     
- Misses      20621    20648      +27

Files with missing lines	Coverage Δ
daft/dataframe/dataframe.py	`85.51% <ø> (+0.11%)`	⬆️
daft/logical/builder.py	`90.65% <100.00%> (+0.37%)`	⬆️
src/daft-dsl/src/lib.rs	`100.00% <ø> (ø)`
src/daft-local-plan/src/translate.rs	`96.69% <100.00%> (+0.06%)`	⬆️
src/daft-logical-plan/src/builder/mod.rs	`91.61% <100.00%> (-0.06%)`	⬇️
src/daft-logical-plan/src/display.rs	`98.02% <100.00%> (-0.04%)`	⬇️
src/daft-logical-plan/src/lib.rs	`100.00% <100.00%> (ø)`
...lan/src/optimization/rules/eliminate_cross_join.rs	`88.09% <ø> (ø)`
...lan/src/optimization/rules/filter_null_join_key.rs	`98.62% <100.00%> (-0.06%)`	⬇️
...al-plan/src/optimization/rules/push_down_filter.rs	`97.35% <ø> (ø)`
... and 13 more

... and 7 files with indirect coverage changes

kevinzwang · 2025-01-22T00:48:28Z

daft/daft/__init__.pyi

-        null_equals_nulls: list[bool] | None,
        how: JoinType,
+        null_equals_nulls: list[bool] | None = None,


small drive-by fix in typing to match rust definition

kevinzwang · 2025-01-22T01:20:29Z

src/daft-dsl/src/join.rs

+
+use crate::{deduplicate_expr_names, ExprRef};
+
+pub fn get_common_join_cols<'a>(


Common columns now determined via schema instead of join keys so that join keys can be modified (ex: casting to supertype) without side effects. This does introduce a small API change in the order of the join schema: common columns are now sorted by left side schema instead of by join keys. I think this is fine but we could introduce a project under the left side to reorder if necessary.

universalmind303

some initial comments. Will get to a more thorough review tomorrow!

src/daft-logical-plan/src/ops/join.rs

src/arrow2/src/array/dyn_ord.rs

universalmind303 · 2025-01-22T03:23:10Z

daft/daft/__init__.pyi

-        join_prefix: str | None = None,
-        join_suffix: str | None = None,
+        join_strategy: JoinStrategy | None = None,
+        column_renaming_params: JoinColumnRenamingParams | None = None,


i'd prefer deprecating join_prefix and join_suffix instead of just flat out removing them.

i'm kind of also on the fence of leaving these as is for the dataframe api. IMO, it's a bit cleaner to do

df.join(df2, prefix="df2.", suffix="_joined")

instead of

from daft.daft import JoinColumnRenamingParams df.join(df2, column_renaming_params=JoinColumnRenamingParams(prefix="df2.", suffix="_joined"))

The API for the actual dataframe operation has not changed, this is just for the builder. Do you think we should be concerned about breaking the builder API?

src/daft-logical-plan/src/ops/join.rs

src/daft-sql/src/lib.rs

daft/daft/__init__.pyi

universalmind303 · 2025-01-29T16:24:04Z

src/daft-dsl/src/join.rs

+    let left_on = deduplicate_expr_names(&left_on);
+    let right_on = deduplicate_expr_names(&right_on);


nit: can we make deduplicate_expr_names take in an iter so we don't have to materialize twice?

We need to materialize here anyway to get the DaftResult out of the iterator as well as split the iterator into two vecs.

fix: joining on different types

4aa0032

kevinzwang changed the title ~~fix: joining on different types~~ fix(join): joining on different types Jan 21, 2025

github-actions bot added the fix label Jan 21, 2025

kevinzwang added 3 commits January 21, 2025 15:00

fix test errors

a4d8eb6

fix test

767e697

Merge branch 'main' into kevin/fix-join-diff-types

c4e3380

fix test

01f53a9

kevinzwang commented Jan 22, 2025

View reviewed changes

add test

3dc2398

kevinzwang commented Jan 22, 2025

View reviewed changes

kevinzwang marked this pull request as ready for review January 22, 2025 01:21

kevinzwang requested review from samster25, colin-ho and universalmind303 January 22, 2025 01:22

universalmind303 reviewed Jan 22, 2025

View reviewed changes

kevinzwang added 2 commits January 28, 2025 12:13

review fixes

91b4aae

Merge branch 'main' into kevin/fix-join-diff-types

fbbdac7

kevinzwang requested a review from universalmind303 January 28, 2025 20:17

universalmind303 approved these changes Jan 29, 2025

View reviewed changes

move joinoptions out of python

449742e

kevinzwang enabled auto-merge (squash) January 29, 2025 19:09

kevinzwang merged commit d00e444 into main Jan 29, 2025
40 of 41 checks passed

kevinzwang deleted the kevin/fix-join-diff-types branch January 29, 2025 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(join): joining on different types #3716

fix(join): joining on different types #3716

kevinzwang commented Jan 21, 2025 •

edited

Loading

codspeed-hq bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

kevinzwang Jan 22, 2025 •

edited

Loading

kevinzwang Jan 22, 2025 •

edited

Loading

universalmind303 left a comment

universalmind303 Jan 22, 2025

universalmind303 Jan 22, 2025

kevinzwang Jan 27, 2025

universalmind303 Jan 29, 2025

kevinzwang Jan 29, 2025


		use crate::{deduplicate_expr_names, ExprRef};

		pub fn get_common_join_cols<'a>(

		let left_on = deduplicate_expr_names(&left_on);
		let right_on = deduplicate_expr_names(&right_on);

fix(join): joining on different types #3716

fix(join): joining on different types #3716

Conversation

kevinzwang commented Jan 21, 2025 • edited Loading

codspeed-hq bot commented Jan 21, 2025 • edited Loading

CodSpeed Performance Report

Merging #3716 will degrade performances by 44.76%

Summary

Benchmarks breakdown

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

kevinzwang Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

kevinzwang Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

universalmind303 left a comment

Choose a reason for hiding this comment

universalmind303 Jan 22, 2025

Choose a reason for hiding this comment

universalmind303 Jan 22, 2025

Choose a reason for hiding this comment

kevinzwang Jan 27, 2025

Choose a reason for hiding this comment

universalmind303 Jan 29, 2025

Choose a reason for hiding this comment

kevinzwang Jan 29, 2025

Choose a reason for hiding this comment

kevinzwang commented Jan 21, 2025 •

edited

Loading

codspeed-hq bot commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

kevinzwang Jan 22, 2025 •

edited

Loading

kevinzwang Jan 22, 2025 •

edited

Loading