Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: change the sort merge join emission as incremental #13894

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

berkaysynnada
Copy link
Contributor

@berkaysynnada berkaysynnada commented Dec 24, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

Since SortMergeJoin appears only when the inputs are sorted, marking its emission type as incremental would reflect its behavior better.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the physical-expr Physical Expressions label Dec 24, 2024
Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

LGTM - this is a bugfix as SMJ always emits incrementally

@alamb
Copy link
Contributor

alamb commented Dec 24, 2024

I wonder if there is some way / reason to test this (I realize it just a property, I was worried it might be broken in the future)

@alamb alamb merged commit 94f08ff into apache:main Dec 24, 2024
25 checks passed
@berkaysynnada
Copy link
Contributor Author

I wonder if there is some way / reason to test this (I realize it just a property, I was worried it might be broken in the future)

AFAIK SMJ is not very stable and it is getting update frequently. Once it becomes stabilized, then it will be more meaningful to assign a property based on a more comprehensive analysis (perhaps it may behave like both incremental and final)

@alamb
Copy link
Contributor

alamb commented Dec 25, 2024

I wonder if there is some way / reason to test this (I realize it just a property, I was worried it might be broken in the future)

AFAIK SMJ is not very stable and it is getting update frequently. Once it becomes stabilized, then it will be more meaningful to assign a property based on a more comprehensive analysis (perhaps it may behave like both incremental and final)

Maybe we can figure out how to write a test that mimics whatever you are doing with these properties (e.g. run a plan end to end that requires the plan to emit data incrementally or something) 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants