fix(databricks): lazily create memtable volume at first use#11962
Open
NickCrews wants to merge 1 commit intoibis-project:mainfrom
Open
fix(databricks): lazily create memtable volume at first use#11962NickCrews wants to merge 1 commit intoibis-project:mainfrom
NickCrews wants to merge 1 commit intoibis-project:mainfrom
Conversation
Fixes ibis-project#11598 Previously, the memtable volume was created eagerly in _post_connect() immediately after connecting, which caused read-only users to fail with a PermissionDenied error even when they never intended to use memtables. This change defers volume creation until the first memtable is actually registered via _register_in_memory_table(), allowing read-only users to connect and query existing data without requiring CREATE VOLUME privilege.
42b386f to
4429f4c
Compare
1 task
Contributor
Author
|
@VENKATASAI1994 what do you think of this alternate formulation? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11598
Previously, the memtable volume was created eagerly in _post_connect() immediately after connecting, which caused read-only users to fail with a PermissionDenied error even when they never intended to use memtables.
This change defers volume creation until the first memtable is actually registered via _register_in_memory_table(), allowing read-only users to connect and query existing data without requiring CREATE VOLUME privilege.
This is a redo of #11956,
where I take a more complete approach:
memtable_volumeparam is interpreted. Before, it was interpreted as just the suffix to a larger path that was generated, eg if you passedfoothen the memtables would actually be created at/Volumes/the_current_catalog_at_init_time/the_current_database_at_init_time/foo. After this change, you just get the plainfoo, so the called has much more control over what they want the volume to be.I didn't add tests because I couldn't find a way to easily create a readonly connection. Perhaps we could create a new role in our databricks account that we use for testing? But mostly I treat this as a refactor, so if existing tests don't begin failing, that's good enough for me.