Skip to content

Conversation

@Centril
Copy link
Contributor

@Centril Centril commented Apr 22, 2024

Description of Changes

Implements Eq + PartialEq for RowRef<'a>.
The impl is unused but will be used in #1107.
Separated out for review purposes.

API and ABI breaking changes

None

Expected complexity level and risk

2, some unsafe, but very local and fairly easy to understand.

Copy link
Contributor

@kim kim left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

Are there any actual uses for which the address equality assumption does not hold, and thus the worse performance could come as a surprise?

@Centril
Copy link
Contributor Author

Centril commented Apr 22, 2024

Are there any actual uses for which the address equality assumption does not hold, and thus the worse performance could come as a surprise?

This impl isn't used in this path currently, but if we used it in find_same_row then we'd end up with committed_table and tx_table having the same layout but not being address equal. This could be solved, if determined to be beneficial, by avoiding a deep clone of row layouts in Table::clone_structure by using Arc<ProductTypeLayout>.

@Centril Centril requested a review from gefjon April 22, 2024 15:43
@bfops bfops added release-any To be landed in any release window no runtime change This change does not affect the final binaries and removed no runtime change This change does not affect the final binaries labels Apr 22, 2024
@Centril Centril added this pull request to the merge queue Apr 25, 2024
Merged via the queue into master with commit 516dfe3 Apr 25, 2024
@Centril Centril deleted the centril/rowref-eq branch April 25, 2024 07:11
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.

4 participants