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

Rework index binding #11867

Merged
merged 20 commits into from May 13, 2024
Merged

Rework index binding #11867

merged 20 commits into from May 13, 2024

Conversation

Maxxen
Copy link
Member

@Maxxen Maxxen commented Apr 29, 2024

This PR reworks the way indexes are bound.

We now lazily bind all indexes (even the built in ART index type) when first accessed instead of initializing/loading them when loading a database. A key change is that the index class itself is now split into UnboundIndex (replacing the UnknownIndex class) and BoundIndex (functioning as the parent class/interface for all concrete instantiated index types) child classes. This unifies the handling of the ART index and custom extension indexes.

Additionally, this PR also adds a no_extension_load optional parameter to the sqllogic unittesters restart command, which prevents previously require'd extensions from being loaded after a restart within a test, making it easier to write tests for extension indexes that verify that a database with a custom index type can be initialized. I have some tests for this in vss that I will push soon, but they're not included in this PR since (afaik) I can't add new tests (or new files in general) through patches.

Also changes the vss extension config to not link so that we actually test loading the extension index properly.

Closes https://github.com/duckdblabs/duckdb-internal/issues/1892

@Maxxen Maxxen requested a review from Mytherin April 29, 2024 14:45
Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks great - some minor comments:

src/function/table/table_scan.cpp Outdated Show resolved Hide resolved
src/include/duckdb/storage/table/table_index_list.hpp Outdated Show resolved Hide resolved
src/storage/table/row_group_collection.cpp Show resolved Hide resolved
src/storage/local_storage.cpp Outdated Show resolved Hide resolved
@duckdb-draftbot duckdb-draftbot marked this pull request as draft April 30, 2024 11:44
@Maxxen Maxxen marked this pull request as ready for review April 30, 2024 13:35
@Maxxen
Copy link
Member Author

Maxxen commented Apr 30, 2024

Ive adapted to your feedback and done some more refactoring moving members between the index/unboundindex/boundindex types. The UnboundIndex now takes ownership of the entire CreateInfo and I've added abstract getters to the index class so that it doesn't have to fields such as name or constraint type separately when unbound.

@Mytherin
Copy link
Collaborator

Mytherin commented May 1, 2024

Could you have a look at the Force Restart CI? It looks like there might be a hang/deadlock there

@Maxxen
Copy link
Member Author

Maxxen commented May 1, 2024

I can't reproduce it locally, but I did find get another hang in another alter test. The issue is that in CatalogSet::AlterEntry we lock the catalog, but later when binding the index we need to be able to access it so that we can add the base table to the bind context...

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 1, 2024 11:12
@Maxxen Maxxen marked this pull request as ready for review May 1, 2024 11:13
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 6, 2024 14:52
@Maxxen Maxxen marked this pull request as ready for review May 7, 2024 11:17
@Maxxen
Copy link
Member Author

Maxxen commented May 7, 2024

It passed my local CI now after merging with main and getting Marks concurrency patches, lets see if it passes here too.

@taniabogatsch taniabogatsch self-requested a review May 7, 2024 13:40
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 10, 2024 11:01
@Maxxen Maxxen marked this pull request as ready for review May 10, 2024 11:43
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 13, 2024 12:26
@Maxxen Maxxen marked this pull request as ready for review May 13, 2024 12:26
@Mytherin
Copy link
Collaborator

Thanks for the changes! LGTM

@Mytherin Mytherin merged commit 2b476eb into duckdb:main May 13, 2024
41 checks passed
github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request May 13, 2024
Merge pull request duckdb/duckdb#11867 from Maxxen/index-binding
Merge pull request duckdb/duckdb#11948 from lnkuiper/json_structure_fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants