Skip to content

Bug fix: Fix case where a post join condition could cause an early exit of the loop #711

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

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

Ulimo
Copy link
Contributor

@Ulimo Ulimo commented Mar 11, 2025

No description provided.

@Ulimo Ulimo requested a review from Copilot March 11, 2025 13:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses a bug where a post join condition could cause an early exit of the loop. It fixes the loop handling logic in the merge join operator and introduces test cases to ensure that all rows are properly checked.

  • Updated the column store merge join logic to continue iterating rather than breaking out early.
  • Added helper methods for managing project and project member entities in the test infrastructure.
  • Introduced a new acceptance test to validate that the post join condition checks all rows.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/FlowtideDotNet.AcceptanceTests/JoinTests.cs Added a new test (PostJoinConditionShouldCheckAllRows) to validate join logic.
tests/FlowtideDotNet.AcceptanceTests/Internal/DatasetGenerator.cs Added helper methods for managing projects and project members.
tests/FlowtideDotNet.AcceptanceTests/FlowtideAcceptanceBase.cs Extended test base with project-related helper methods.
tests/FlowtideDotNet.AcceptanceTests/Internal/FlowtideTestStream.cs Added project-related methods to delegate to generator functions.
src/FlowtideDotNet.Core/Operators/Join/MergeJoin/ColumnStoreMergeJoin.cs Modified loop control in join methods to prevent premature loop exit.
Comments suppressed due to low confidence (2)

src/FlowtideDotNet.Core/Operators/Join/MergeJoin/ColumnStoreMergeJoin.cs:233

  • Replacing 'break' with 'continue' fixes the early exit issue. Please verify that omitting the reset of _searchRightComparer.end does not affect subsequent iterations.
continue;

src/FlowtideDotNet.Core/Operators/Join/MergeJoin/ColumnStoreMergeJoin.cs:438

  • Replacing 'break' with 'continue' ensures that all rows are processed. Verify that not resetting _searchLeftComparer.end does not introduce unintended side effects.
continue;

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Benchmark

Benchmark suite Current: 04bad67 Previous: 22d4209 Ratio
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.InnerJoin 414125361.1111111 ns (± 9443979.677345304) 458826533.3333333 ns (± 10640413.19005047) 0.90
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.LeftJoin 502283150 ns (± 8758157.513834909) 564994640 ns (± 28550338.329818636) 0.89
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ProjectionAndNormalization 148388740 ns (± 16319550.236122727) 169842820 ns (± 7144067.768715523) 0.87
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.SumAggregation 148417772.2222222 ns (± 14350855.899107357) 171581250 ns (± 12415362.812660772) 0.86
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ListAggWithMapAggregation 1899091310 ns (± 99970537.61284094) 1982296640 ns (± 123349811.75190067) 0.96

This comment was automatically generated by workflow using github-action-benchmark.

@Ulimo Ulimo merged commit 5873845 into main Mar 11, 2025
7 checks passed
@Ulimo Ulimo deleted the bugfix_fix_mergejoinpostcondition branch March 11, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant