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

Implement RelValue: Eq + Hash #1107

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Implement RelValue: Eq + Hash #1107

merged 3 commits into from
Apr 30, 2024

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Apr 17, 2024

Description of Changes

The main goal of this PR is to make RelValues directly usable in the IncrementalJoin algorithm's HashSet, therebly avoiding forcing everything to Cow<'_, PV>, which is useful in some cases. To do this, RelValue must implement Eq + Hash and to do that, PV and RowRef must be consistent in terms of equality and hash functions. The latter is achieved in a series of PRs upon which this PR is based. Having RowRef: PartialEq<PV> and consistent hashes is also useful for the insert and delete paths.

  1. impl Eq + Hash for RelValue
  2. Use the above in incremental evaluation.

Based on #1164.

Fixes #1119.

API and ABI breaking changes

None

Expected complexity level and risk

2

Testing

Proptests were added for the new code in previous PRs.

@Centril Centril force-pushed the centril/relvalue-hash branch 4 times, most recently from adcb1c8 to 588f175 Compare April 18, 2024 14:44
@Centril Centril changed the base branch from master to centril/bsatn-hash-eq April 18, 2024 14:44
@Centril Centril force-pushed the centril/relvalue-hash branch 5 times, most recently from 03f23d3 to 28de797 Compare April 22, 2024 11:24
Base automatically changed from centril/bsatn-hash-eq to master April 25, 2024 01:11
@Centril Centril changed the title [WIP] Centril/relvalue hash Implement RelValue: Eq + Hash, RowRef: Hash + PartialEq<ProductValue> Apr 26, 2024
@Centril Centril marked this pull request as ready for review April 26, 2024 00:34
@Centril Centril changed the base branch from master to centril/row-ref-hash April 26, 2024 08:43
@Centril Centril changed the base branch from centril/row-ref-hash to centril/eq-pv-row-ref April 26, 2024 08:50
@Centril Centril changed the title Implement RelValue: Eq + Hash, RowRef: Hash + PartialEq<ProductValue> Implement RelValue: Eq + Hash Apr 26, 2024
@bfops bfops added the release-any To be landed in any release window label Apr 29, 2024
@cloutiertyler
Copy link
Contributor

Could we get the motivation for this PR in the description?

@Centril
Copy link
Contributor Author

Centril commented Apr 29, 2024

Could we get the motivation for this PR in the description?

There ya go :)

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Style nits. Functionality looks good, I just want to use words to describe things rather than letters.

crates/core/src/host/module_host.rs Outdated Show resolved Hide resolved
crates/core/src/host/module_host.rs Outdated Show resolved Hide resolved
crates/core/src/host/module_host.rs Outdated Show resolved Hide resolved
@Centril Centril reopened this Apr 30, 2024
Base automatically changed from centril/eq-pv-row-ref to master April 30, 2024 21:37
@Centril Centril enabled auto-merge April 30, 2024 21:58
@Centril Centril added this pull request to the merge queue Apr 30, 2024
Merged via the queue into master with commit 0142e14 Apr 30, 2024
6 checks passed
@Centril Centril deleted the centril/relvalue-hash branch April 30, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants