Skip to content

feat: add PhysicalJoinType - #572

Closed
danepitkin wants to merge 4 commits into
substrait-io:mainfrom
danepitkin:danepitkin/physicaljointype
Closed

feat: add PhysicalJoinType#572
danepitkin wants to merge 4 commits into
substrait-io:mainfrom
danepitkin:danepitkin/physicaljointype

Conversation

@danepitkin

@danepitkin danepitkin commented Nov 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

BREAKING CHANGE: the JoinType for physical operators HashJoin, MergeJoin, and
NestedLoopJoin has been refactored into a shared enum called PhysicalJoinType.
It is wire compatible and a new is_null_aware field is added to the physical join rels.

Closes #563

@danepitkin

danepitkin commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Question: Is it better for the original ANTI_JOIN enums to default to NO_NULLS or WITH_NULLs?

EpsilonPrime
EpsilonPrime reviewed Nov 8, 2023
View reviewed changes

@EpsilonPrime EpsilonPrime left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is null handling part of the type or is it a separate setting/option?

Comment thread proto/substrait/algebra.proto
JOIN_TYPE_LEFT_ANTI = 7;
JOIN_TYPE_RIGHT_ANTI = 8;
}
PhysicalJoinType type = 7;

@EpsilonPrime EpsilonPrime Nov 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For protobuf compatibility it is better to make this 8 and mark 7 as deprecated.

@EpsilonPrime EpsilonPrime Nov 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose if we keep all of the values the same as you've done here it would be wire compatible which mean we wouldn't need to renumber the field for the "type" change.

@danepitkin danepitkin Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's still technically wire compatible! The Anti Join will have to default to one of the two options though.

@danepitkin danepitkin Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe I change this to be

  PHYSICAL_JOIN_TYPE_LEFT_ANTI = 7;
  PHYSICAL_JOIN_TYPE_RIGHT_ANTI = 8;
  PHYSICAL_JOIN_TYPE_LEFT_ANTI_NULL_AWARE = 9;
  PHYSICAL_JOIN_TYPE_RIGHT_ANTI_NULL_AWARE = 10;

?

@danepitkin danepitkin Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to this ^

@danepitkin danepitkin Nov 28, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated again to use a separate is_null_aware field in the rels

@danepitkin

danepitkin commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Is null handling part of the type or is it a separate setting/option?

This Velox documentation makes me think that these are considered different types of anti joins (rather than a separate config): https://facebookincubator.github.io/velox/develop/anti-join.html

@danepitkin

danepitkin commented Nov 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hmm, although they did implement it slightly differently, using JoinType::kAnti and JoinType::kNullAwareAnti, presumably with the NULL config separate.

Edit: actually, this is similar to this PR, but the left/right option is configured elsewhere it seems.

@github-actions

github-actions Bot commented Nov 8, 2023

Copy link
Copy Markdown

ACTION NEEDED

Substrait follows the Conventional Commits
specification for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@danepitkin

danepitkin commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

I see @westonpace 's old PR here does use a null_is_match bool instead of expanding the jointype enum #467

@msirek msirek mentioned this pull request Nov 16, 2023
Closed
@danepitkin

danepitkin commented Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Hmm, maybe it would be better to have bool null_aware in each physical join so that other join types can use it.

@jacques-n

jacques-n commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

This shouldn't be a breaking binary change, right?

@danepitkin

danepitkin commented Nov 28, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This shouldn't be a breaking binary change, right?

Yes, good call!

Edit: The "Breaking Changes Check" does flag this, though.

@danepitkin
danepitkin force-pushed the danepitkin/physicaljointype branch from f8bda5d to a65e4ce Compare November 28, 2023 21:37
vbarua
vbarua reviewed Dec 20, 2023
View reviewed changes
Comment thread proto/substrait/algebra.proto
JOIN_TYPE_RIGHT_ANTI = 8;
}
PhysicalJoinType type = 7;
bool is_null_aware = 8;

@vbarua vbarua Dec 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What exactly does this boolean enable? It would be good to document it.

@westonpace

westonpace commented Jan 3, 2024

Copy link
Copy Markdown
Member

I've created a new PR specifically for dealing with the null_is_match / null_aware stuff at #585

Do you want to rescope this PR to just dealing with the join type?

@westonpace westonpace added awaiting-user-input This issue is waiting on further input from users and removed awaiting PMC approval labels Jan 23, 2024
@jacques-n

jacques-n commented Aug 8, 2024

Copy link
Copy Markdown
Contributor

No progress has been made in more than six months. Closing without prejudice.

@jacques-n jacques-n closed this Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vbarua vbarua vbarua left review comments
@EpsilonPrime EpsilonPrime EpsilonPrime approved these changes
@jacques-n jacques-n Awaiting requested review from jacques-n jacques-n is a code owner
@cpcloud cpcloud Awaiting requested review from cpcloud cpcloud is a code owner
@westonpace westonpace Awaiting requested review from westonpace westonpace is a code owner

Assignees

No one assigned

Labels

awaiting-user-input This issue is waiting on further input from users

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor JoinType for physical operators

5 participants

@danepitkin @jacques-n @westonpace @EpsilonPrime @vbarua