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

refactor: fix membership check #510

Merged
merged 2 commits into from
Jan 29, 2025
Merged

refactor: fix membership check #510

merged 2 commits into from
Jan 29, 2025

Conversation

iajoiner
Copy link
Contributor

@iajoiner iajoiner commented Jan 28, 2025

Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

Please go through the following checklist

Rationale for this change

We want to let membership check gadget take care of multiplicity computation to reduce the amount of duplicates in the sort merge join code.

What changes are included in this PR?

See above.

Are these changes tested?

Yes.

Sorry, something went wrong.

@iajoiner iajoiner force-pushed the refactor/membership branch 4 times, most recently from abf3ba9 to 8b22a0b Compare January 29, 2025 16:32
@iajoiner iajoiner requested a review from JayWhite2357 January 29, 2025 16:32
@iajoiner iajoiner force-pushed the refactor/membership branch from 8b22a0b to 9771a0e Compare January 29, 2025 16:45
@iajoiner iajoiner enabled auto-merge January 29, 2025 16:59
@@ -33,15 +57,23 @@ pub(crate) fn final_round_evaluate_membership_check<'a, S: Scalar>(
beta: S,
columns: &[Column<'a, S>],
candidate_subset: &[Column<'a, S>],
multiplicities: &'a [i128],
input_ones: &'a [bool],
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: We still should have input_ones be an argument to mirror verifier_evaluate.

Comment on lines 74 to 75
let multiplicities = get_multiplicities::<S>(candidate_subset, columns);
let alloc_multiplicities = alloc.alloc_slice_copy(multiplicities.as_slice());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always done? Should get_multiplicities simply accept alloc?

@iajoiner iajoiner requested a review from JayWhite2357 January 29, 2025 17:28
@iajoiner iajoiner mentioned this pull request Jan 29, 2025
8 tasks
@iajoiner iajoiner merged commit ae57889 into main Jan 29, 2025
12 checks passed
Copy link

🎉 This PR is included in version 0.71.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@iajoiner iajoiner deleted the refactor/membership branch January 31, 2025 21:02
iajoiner added a commit that referenced this pull request Feb 5, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Please be sure to look over the pull request guidelines here:
https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.
Depends on the following PRs:
- [x] #430
- [x] #482 
- [x] #503
- [x] #506
- [x] #509 
- [x] #510

Closes #394.
# Please go through the following checklist
- [x] The PR title and commit messages adhere to guidelines here:
https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md.
In particular `!` is used if and only if at least one breaking change
has been introduced.
- [x] I have run the ci check script with `source
scripts/run_ci_checks.sh`.

# Rationale for this change
We need to add sort merge inner joins.
<!--
Why are you proposing this change? If this is already explained clearly
in the linked issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.

 Example:
 Add `NestedLoopJoinExec`.
 Closes #345.

Since we added `HashJoinExec` in #323 it has been possible to do
provable inner joins. However performance is not satisfactory in some
cases. Hence we need to fix the problem by implement
`NestedLoopJoinExec` and speed up the code
 for `HashJoinExec`.
-->

# What changes are included in this PR?
- add `SortMergeJoinExec`
- add `Column::rho` and `Table::add_rho_column`
<!--
There is no need to duplicate the description in the ticket here but it
is sometimes worth providing a summary of the individual changes in this
PR.

Example:
- Add `NestedLoopJoinExec`.
- Speed up `HashJoinExec`.
- Route joins to `NestedLoopJoinExec` if the outer input is sufficiently
small.
-->

# Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?

Example:
Yes.
-->
Will be.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants