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!: merge result_evaluate back to first_round_evaluate #404

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

iajoiner
Copy link
Contributor

@iajoiner iajoiner commented Dec 4, 2024

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

Since we are adding range check which will require FirstRoundBuilder to read query results we partly undo #257 and merge ProofPlan::result_evaluate back into ProofPlan::first_round_evaluate.

What changes are included in this PR?

See above.

Are these changes tested?

Yes.

@iajoiner iajoiner requested a review from JayWhite2357 December 4, 2024 01:26
@iajoiner iajoiner force-pushed the refactor/first-round branch from 53cc8fd to 31a811e Compare December 4, 2024 01:32
@iajoiner iajoiner enabled auto-merge December 4, 2024 01:32
@iajoiner iajoiner force-pushed the refactor/first-round branch from 31a811e to e632e7d Compare December 4, 2024 01:41
@iajoiner iajoiner force-pushed the refactor/first-round branch from e632e7d to 2d9fd7c Compare December 4, 2024 01:44
@iajoiner iajoiner mentioned this pull request Dec 4, 2024
5 tasks
Copy link
Contributor

@JayWhite2357 JayWhite2357 left a comment

Choose a reason for hiding this comment

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

This should be done for ProofExpr as well.

&self,
builder: &mut FirstRoundBuilder,
alloc: &'a Bump,
table_map: &IndexMap<TableRef, Table<'a, S>>,
) -> (Table<'a, S>, Vec<usize>);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT:
One of the motivations for doing this was so that the lengths could be part of the builder rather than an output.

@iajoiner iajoiner merged commit 078191c into main Dec 4, 2024
11 checks passed
Copy link

github-actions bot commented Dec 4, 2024

🎉 This PR is included in version 0.55.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

iajoiner added a commit that referenced this pull request Dec 4, 2024
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
- [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`.
- The following upstream PRs have been merged:
  - [x] #381
  - [x] #401
  - [x] #404

# Rationale for this change
This PR replaces #121 and is designed to test whether our `ProofPlan`s
are truly composable now.
<!--
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 `SliceExec`.
<!--
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.
-->
Yes.
@iajoiner iajoiner deleted the refactor/first-round branch December 4, 2024 18:58
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.

2 participants