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
Rework index binding #11867
Conversation
There was a problem hiding this 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:
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. |
Could you have a look at the Force Restart CI? It looks like there might be a hang/deadlock there |
I can't reproduce it locally, but I did find get another hang in another alter test. The issue is that in |
It passed my local CI now after merging with main and getting Marks concurrency patches, lets see if it passes here too. |
Thanks for the changes! LGTM |
Merge pull request duckdb/duckdb#11867 from Maxxen/index-binding Merge pull request duckdb/duckdb#11948 from lnkuiper/json_structure_fix
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 theUnknownIndex
class) andBoundIndex
(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 unittestersrestart
command, which prevents previouslyrequire
'd extensions from being loaded after arestart
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 invss
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