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

Stop generating object keys from primary key value hashes #4708

Merged
merged 14 commits into from
May 27, 2021
Merged

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented May 25, 2021

What, How & Why?

Fixes #4522

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)

@jedelbo jedelbo requested a review from finnschiermer May 25, 2021 08:15
Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

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

Don't we need tests validating file format upgrades from v11 and v21 ?

@@ -346,7 +347,7 @@ class Table {
size_t get_index_in_group() const noexcept;
TableKey get_key() const noexcept;

uint32_t allocate_sequence_number();
uint64_t allocate_sequence_number();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

@@ -3248,12 +3248,15 @@ TEMPLATE_TEST_CASE("results: get<Obj>() intermixed with writes", "", ResultsFrom

r->begin_transaction();

/*
* We no longer insert based on pk. Objects will always be inserted at the end.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just remove this code completely?

@@ -76,6 +76,7 @@ TEST(GlobalKey_Compare)
CHECK_LESS(GlobalKey(0, 0), GlobalKey(1, 0));
}

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we just remove this code?

Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

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

Looking better... but...see comments


SHARED_GROUP_TEST_PATH(temp_copy);

// Make a copy of the version 9 database so that we keep the
Copy link
Contributor

Choose a reason for hiding this comment

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

version 20 ?

src/realm/sync/noinst/client_reset.cpp Show resolved Hide resolved
Copy link
Contributor

@finnschiermer finnschiermer left a comment

Choose a reason for hiding this comment

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

Epic!

@jedelbo jedelbo merged commit e4de40e into v11 May 27, 2021
@jedelbo jedelbo deleted the je/faster-pk branch May 27, 2021 12:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants