implement DuckDB::InstanceCache#get_or_create#939
Conversation
- `DuckDB::InstanceCache#get_or_create` is wrapping C-API `duckdb_get_or_create_from_cache`.
WalkthroughThis pull request adds a new function to create a Ruby database object from an existing DuckDB handle and implements a method to retrieve or create a DuckDB database instance from a cache with optional path and configuration arguments. It introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant RubyApp
participant rbduckdb_create_database_obj
participant Allocator
participant RubyDuckDB
RubyApp->>rbduckdb_create_database_obj: Call with duckdb_database handle
rbduckdb_create_database_obj->>Allocator: Allocate cDuckDBDatabase object
Allocator-->>rbduckdb_create_database_obj: Return allocated object
rbduckdb_create_database_obj->>RubyDuckDB: Retrieve RubyDuckDB structure
rbduckdb_create_database_obj->>RubyDuckDB: Assign provided duckdb_database to db field
rbduckdb_create_database_obj->>RubyApp: Return new Ruby DB object
sequenceDiagram
participant RubyApp
participant InstanceCache
participant ArgumentProcessor
participant DuckDBCache
RubyApp->>InstanceCache: get_or_create(vpath, vconfig)
InstanceCache->>ArgumentProcessor: Validate & parse arguments
ArgumentProcessor->>DuckDBCache: Request DB instance (or create)
DuckDBCache-->>ArgumentProcessor: Return DB instance or error
ArgumentProcessor-->>InstanceCache: Pass result or raise error
InstanceCache->>RubyApp: Return DB object
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (17)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ext/duckdb/database.c(1 hunks)ext/duckdb/database.h(1 hunks)ext/duckdb/instance_cache.c(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ext/duckdb/instance_cache.c (2)
ext/duckdb/database.h (1)
rbduckdb_create_database_obj(11-11)ext/duckdb/database.c (1)
rbduckdb_create_database_obj(121-127)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (3.4.1, 1.2.2)
- GitHub Check: test (3.2.6, 1.1.3)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (head, 1.2.2)
- GitHub Check: test (mswin, 1.1.1)
- GitHub Check: test (ucrt, 1.1.3)
- GitHub Check: test (ucrt, 1.1.1)
- GitHub Check: test (ucrt, 1.2.2)
- GitHub Check: test (mingw, 1.1.1)
- GitHub Check: test (3.4.1, 1.1.1)
- GitHub Check: test (mingw, 1.2.2)
- GitHub Check: test (3.2.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.1.6, 1.2.2)
- GitHub Check: test (3.2.6, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.3)
- GitHub Check: test (3.1.6, 1.2.2)
🔇 Additional comments (4)
ext/duckdb/database.h (1)
11-11: Clean function declaration for database object creation.This new function declaration follows the existing coding style and naming conventions. It will allow creating Ruby database objects from existing DuckDB handles, which complements the new instance cache functionality.
ext/duckdb/database.c (1)
121-127: Good implementation of database object creation.The implementation correctly allocates a new Ruby object, associates it with the provided DuckDB database handle, and returns the object. This function follows the established patterns in the codebase.
ext/duckdb/instance_cache.c (2)
10-10: Appropriate function prototype for get_or_create method.The function signature correctly uses the variable arguments approach (int argc, VALUE *argv) which is consistent with Ruby's C extension conventions.
101-101: Correct method registration for get_or_create.The registration properly uses
-1to indicate variable arguments, which matches the implementation.
| static VALUE duckdb_instance_cache_get_or_create(int argc, VALUE *argv, VALUE self) { | ||
| VALUE vpath = Qnil; | ||
| VALUE vconfig = Qnil; | ||
| const char *path = NULL; | ||
| char *error = NULL; | ||
| duckdb_config config = NULL; | ||
| duckdb_database db; | ||
| rubyDuckDBInstanceCache *ctx; | ||
|
|
||
| rb_scan_args(argc, argv, "02", &vpath, &vconfig); | ||
| if (!NIL_P(vpath)) { | ||
| path = StringValuePtr(vpath); | ||
| } | ||
| if (!NIL_P(vconfig)) { | ||
| if (!rb_obj_is_kind_of(vconfig, cDuckDBConfig)) { | ||
| rb_raise(rb_eTypeError, "The second argument must be DuckDB::Config object."); | ||
| } | ||
| rubyDuckDBConfig *ctx_config = get_struct_config(vconfig); | ||
| config = ctx_config->config; | ||
| } | ||
|
|
||
| TypedData_Get_Struct(self, rubyDuckDBInstanceCache, &instance_cache_data_type, ctx); | ||
|
|
||
| if (duckdb_get_or_create_from_cache(ctx->instance_cache, path, &db, config, &error) == DuckDBError) { | ||
| if (error) { | ||
| rb_raise(eDuckDBError, "%s", error); | ||
| } else { | ||
| rb_raise(eDuckDBError, "Failed to get or create database from instance cache"); | ||
| } | ||
| } | ||
| return rbduckdb_create_database_obj(db); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing type checking for path argument and RDoc documentation.
The implementation looks solid overall, but has a few areas for improvement:
- There's no check that
vpathis a String before callingStringValuePtr(). - The method lacks RDoc documentation explaining its purpose and parameters.
- Potential memory leak if
erroris allocated by DuckDB but not freed after use.
Consider adding these improvements:
+/*
+ * call-seq:
+ * instance_cache.get_or_create -> DuckDB::Database
+ * instance_cache.get_or_create(path) -> DuckDB::Database
+ * instance_cache.get_or_create(path, config) -> DuckDB::Database
+ *
+ * Gets an existing database from the instance cache or creates a new one.
+ * The path and config are optional. If provided, path must be a String and
+ * config must be a DuckDB::Config object.
+ */
static VALUE duckdb_instance_cache_get_or_create(int argc, VALUE *argv, VALUE self) {
VALUE vpath = Qnil;
VALUE vconfig = Qnil;
const char *path = NULL;
char *error = NULL;
duckdb_config config = NULL;
duckdb_database db;
rubyDuckDBInstanceCache *ctx;
rb_scan_args(argc, argv, "02", &vpath, &vconfig);
if (!NIL_P(vpath)) {
+ Check_Type(vpath, T_STRING);
path = StringValuePtr(vpath);
}
if (!NIL_P(vconfig)) {
if (!rb_obj_is_kind_of(vconfig, cDuckDBConfig)) {
rb_raise(rb_eTypeError, "The second argument must be DuckDB::Config object.");
}
rubyDuckDBConfig *ctx_config = get_struct_config(vconfig);
config = ctx_config->config;
}
TypedData_Get_Struct(self, rubyDuckDBInstanceCache, &instance_cache_data_type, ctx);
+ if (ctx->instance_cache == NULL) {
+ rb_raise(eDuckDBError, "InstanceCache has been destroyed");
+ }
if (duckdb_get_or_create_from_cache(ctx->instance_cache, path, &db, config, &error) == DuckDBError) {
if (error) {
- rb_raise(eDuckDBError, "%s", error);
+ VALUE message = rb_str_new_cstr(error);
+ duckdb_free(error);
+ rb_raise(eDuckDBError, "%s", StringValuePtr(message));
} else {
rb_raise(eDuckDBError, "Failed to get or create database from instance cache");
}
}
return rbduckdb_create_database_obj(db);
}Note: Please verify that duckdb_free is the correct function to free memory allocated by DuckDB. If there's a different API function for this purpose, use that instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static VALUE duckdb_instance_cache_get_or_create(int argc, VALUE *argv, VALUE self) { | |
| VALUE vpath = Qnil; | |
| VALUE vconfig = Qnil; | |
| const char *path = NULL; | |
| char *error = NULL; | |
| duckdb_config config = NULL; | |
| duckdb_database db; | |
| rubyDuckDBInstanceCache *ctx; | |
| rb_scan_args(argc, argv, "02", &vpath, &vconfig); | |
| if (!NIL_P(vpath)) { | |
| path = StringValuePtr(vpath); | |
| } | |
| if (!NIL_P(vconfig)) { | |
| if (!rb_obj_is_kind_of(vconfig, cDuckDBConfig)) { | |
| rb_raise(rb_eTypeError, "The second argument must be DuckDB::Config object."); | |
| } | |
| rubyDuckDBConfig *ctx_config = get_struct_config(vconfig); | |
| config = ctx_config->config; | |
| } | |
| TypedData_Get_Struct(self, rubyDuckDBInstanceCache, &instance_cache_data_type, ctx); | |
| if (duckdb_get_or_create_from_cache(ctx->instance_cache, path, &db, config, &error) == DuckDBError) { | |
| if (error) { | |
| rb_raise(eDuckDBError, "%s", error); | |
| } else { | |
| rb_raise(eDuckDBError, "Failed to get or create database from instance cache"); | |
| } | |
| } | |
| return rbduckdb_create_database_obj(db); | |
| } | |
| /* | |
| * call-seq: | |
| * instance_cache.get_or_create -> DuckDB::Database | |
| * instance_cache.get_or_create(path) -> DuckDB::Database | |
| * instance_cache.get_or_create(path, config) -> DuckDB::Database | |
| * | |
| * Gets an existing database from the instance cache or creates a new one. | |
| * The path and config are optional. If provided, path must be a String and | |
| * config must be a DuckDB::Config object. | |
| */ | |
| static VALUE duckdb_instance_cache_get_or_create(int argc, VALUE *argv, VALUE self) { | |
| VALUE vpath = Qnil; | |
| VALUE vconfig = Qnil; | |
| const char *path = NULL; | |
| char *error = NULL; | |
| duckdb_config config = NULL; | |
| duckdb_database db; | |
| rubyDuckDBInstanceCache *ctx; | |
| rb_scan_args(argc, argv, "02", &vpath, &vconfig); | |
| if (!NIL_P(vpath)) { | |
| Check_Type(vpath, T_STRING); | |
| path = StringValuePtr(vpath); | |
| } | |
| if (!NIL_P(vconfig)) { | |
| if (!rb_obj_is_kind_of(vconfig, cDuckDBConfig)) { | |
| rb_raise(rb_eTypeError, "The second argument must be DuckDB::Config object."); | |
| } | |
| rubyDuckDBConfig *ctx_config = get_struct_config(vconfig); | |
| config = ctx_config->config; | |
| } | |
| TypedData_Get_Struct(self, rubyDuckDBInstanceCache, &instance_cache_data_type, ctx); | |
| if (ctx->instance_cache == NULL) { | |
| rb_raise(eDuckDBError, "InstanceCache has been destroyed"); | |
| } | |
| if (duckdb_get_or_create_from_cache(ctx->instance_cache, path, &db, config, &error) == DuckDBError) { | |
| if (error) { | |
| VALUE message = rb_str_new_cstr(error); | |
| duckdb_free(error); | |
| rb_raise(eDuckDBError, "%s", StringValuePtr(message)); | |
| } else { | |
| rb_raise(eDuckDBError, "Failed to get or create database from instance cache"); | |
| } | |
| } | |
| return rbduckdb_create_database_obj(db); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/duckdb_test/instance_cache_test.rb (2)
14-30: Well-structured concurrency test, with one suggestion.The test effectively checks multi-threaded access to the same database path through the cache. Good job ensuring proper cleanup in each iteration.
Consider adding an assertion to verify that calling
get_or_createwith the same path returns the same database instance, which would validate the caching behavior.def test_get_or_create cache = DuckDB::InstanceCache.new path = 'test_shared_db.db' 30.times do + db1 = cache.get_or_create(path) + db2 = cache.get_or_create(path) + assert_same db1, db2, "Cache should return same instance for the same path" thread = Thread.new do db = cache.get_or_create(path) assert_instance_of DuckDB::Database, db db.close end db = cache.get_or_create(path) assert_instance_of DuckDB::Database, db db.close thread.join FileUtils.rm_f(path) end end
14-51: Consider adding error case testing.The current tests verify the happy path scenarios, but there's no test for potential error conditions. Consider adding tests for:
- Invalid paths or configurations
- Error handling when the database cannot be created
- Behavior when trying to get a database that was previously closed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/duckdb_test/instance_cache_test.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (head, 1.1.1)
- GitHub Check: test (3.4.1, 1.1.3)
- GitHub Check: test (mingw, 1.1.1)
- GitHub Check: test (mingw, 1.1.3)
- GitHub Check: test (mswin, 1.1.3)
- GitHub Check: test (3.4.1, 1.2.2)
- GitHub Check: test (mswin, 1.1.1)
- GitHub Check: test (ucrt, 1.1.3)
- GitHub Check: test (3.4.1, 1.1.1)
- GitHub Check: test (3.2.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.2.6, 1.1.3)
- GitHub Check: test (3.3.6, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.3)
- GitHub Check: test (3.1.6, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.2.6, 1.2.2)
🔇 Additional comments (4)
test/duckdb_test/instance_cache_test.rb (4)
4-4: Good addition of FileUtils for test cleanup.The addition of the FileUtils module is appropriate for managing the test database file cleanup at line 28.
32-37: Good test for default behavior without path.This test properly verifies that
get_or_createworks correctly when called without any path argument.
39-44: Good edge case test with empty path.Testing with an empty string path is a good edge case to include.
46-51: Good test for in-memory database creation.Testing with the special
:memory:path ensures compatibility with in-memory database functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/duckdb_test/instance_cache_test.rb (3)
14-30: Thread safety testing looks good, but could be enhanced.The test correctly checks basic thread safety by creating threads that access the same database path. However, there's no verification that the same database instance is being returned for the same path, which is a key aspect of the caching functionality.
Consider adding an assertion to verify cache behavior by checking object identity:
def test_get_or_create cache = DuckDB::InstanceCache.new path = 'test_shared_db.db' + first_db = nil 30.times do + # Store the first database instance for comparison + first_db ||= cache.get_or_create(path) + thread = Thread.new do db = cache.get_or_create(path) assert_instance_of DuckDB::Database, db + # Verify we get the same instance from the cache + assert_same first_db, db db.close end db = cache.get_or_create(path) assert_instance_of DuckDB::Database, db + # Verify we get the same instance from the cache + assert_same first_db, db db.close thread.join FileUtils.rm_f(path) end end
53-67: Good comprehensive test for configuration.This test is well-structured and tests important functionality:
- Creates a configuration with non-default settings
- Creates a database with the configuration
- Verifies the configuration is applied by checking query behavior
- Properly cleans up resources
One small suggestion to make this test more robust:
def test_get_or_create_with_config cache = DuckDB::InstanceCache.new config = DuckDB::Config.new config['default_order'] = 'DESC' db = cache.get_or_create(nil, config) con = db.connect con.query('CREATE TABLE numbers (number INTEGER)') con.query('INSERT INTO numbers VALUES (2), (1), (4), (3)') result = con.query('SELECT number FROM numbers ORDER BY number') assert_equal(4, result.first.first) + + # Additional verification to ensure all results are in descending order + expected = [4, 3, 2, 1] + actual = result.map { |row| row.first } + assert_equal expected, actual + con.close db.close cache.destroy end
14-67: Consider standardizing resource cleanup across tests.All tests properly close database connections, but only
test_get_or_createremoves database files and onlytest_get_or_create_with_configcallscache.destroy. Consider standardizing cleanup approach across all tests to ensure consistent resource management.You could add a
setupandteardownmethod to ensure consistent cleanup:def setup @cache = DuckDB::InstanceCache.new @db_path = 'test_shared_db.db' end def teardown FileUtils.rm_f(@db_path) if File.exist?(@db_path) @cache.destroy if @cache endThen update tests to use these instance variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/duckdb_test/instance_cache_test.rb(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test (mswin, 1.2.2)
- GitHub Check: test (ucrt, 1.1.3)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (mingw, 1.1.1)
- GitHub Check: test (mingw, 1.1.3)
- GitHub Check: test (ucrt, 1.1.1)
- GitHub Check: test (ucrt, 1.2.2)
- GitHub Check: test (3.4.1, 1.1.3)
- GitHub Check: test (mingw, 1.2.2)
- GitHub Check: test (3.2.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.4.1, 1.1.1)
- GitHub Check: test (3.2.6, 1.1.3)
- GitHub Check: test (3.2.6, 1.2.2)
- GitHub Check: test (3.3.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.3)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.1.6, 1.2.2)
- GitHub Check: test (head, 1.2.2)
🔇 Additional comments (4)
test/duckdb_test/instance_cache_test.rb (4)
4-4: Adding FileUtils module - good addition.The FileUtils module is required for file management operations, specifically for removing the test database files. This is a necessary addition since the tests will be creating physical database files that need to be cleaned up after testing.
32-37: Good test for default behavior.This test properly verifies that
get_or_createworks correctly when called without a path parameter. It confirms that a valid DuckDB::Database instance is returned and properly cleans up by closing the connection.
39-44: Good test for empty path.This test properly verifies that
get_or_createworks correctly when called with an empty string as the path. It confirms that a valid DuckDB::Database instance is returned and properly closes the connection afterward.
46-51: Good test for in-memory database.The test correctly verifies that
get_or_createworks with the special ':memory:' path parameter which should create an in-memory database. It confirms that a valid DuckDB::Database instance is returned and properly closes the connection afterward.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/duckdb/instance_cache.rb (2)
14-16: Add documentation for the get_or_create methodThe
getmethod calls an undocumentedget_or_createmethod that appears to be implemented in the C extension. Consider adding a comment to indicate this, making the code more maintainable:def get(path: nil, config: nil) + # Calls the C extension method implemented in ext/duckdb/instance_cache.c get_or_create(path, config) endAdditionally, consider adding error handling for potential failures in the C extension method call.
5-18: Consider adding instance variable initializationThe
InstanceCacheclass doesn't appear to have any instance variables or initialization logic. If the class needs to maintain state between calls toget, consider adding an initializer method:class InstanceCache def initialize # Initialize any necessary instance variables end def get(path: nil, config: nil) get_or_create(path, config) end endIf no state is needed, this might be a good candidate for class methods instead of instance methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/duckdb/instance_cache.rb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test (3.4.1, 1.2.2)
- GitHub Check: test (3.1.6, 1.2.2)
- GitHub Check: test (ucrt, 1.2.2)
- GitHub Check: test (mswin, 1.1.1)
- GitHub Check: test (head, 1.1.1)
- GitHub Check: test (3.3.6, 1.2.2)
- GitHub Check: test (head, 1.2.2)
- GitHub Check: test (3.1.6, 1.2.2)
- GitHub Check: test (3.2.7, 1.1.3)
- GitHub Check: test (3.3.6, 1.1.1)
- GitHub Check: test (3.4.1, 1.1.3)
- GitHub Check: test (3.4.1, 1.1.1)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (3.2.6, 1.1.3)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.2.6, 1.2.2)
- GitHub Check: test (3.2.7, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.3)
| if defined?(DuckDB::InstanceCache) | ||
|
|
||
| module DuckDB | ||
| # The DuckDB::InstanceCache is necessary if a client/program (re)opens | ||
| # multiple databases to the same file within the same statement. | ||
| # | ||
| # require 'duckdb' | ||
| # cache = DuckDB::InstanceCache.new | ||
| # db1 = cache.get(path: 'db.duckdb') | ||
| # db2 = cache.get(path: 'db.duckdb') | ||
| class InstanceCache | ||
| def get(path: nil, config: nil) | ||
| get_or_create(path, config) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the conditional block structure
The current implementation wraps the entire module definition in an if defined?(DuckDB::InstanceCache) check. This is counterintuitive, as it means the class will only be defined if it's already defined elsewhere. Typically, this pattern is used to prevent redefinition, but the condition should be negated:
-if defined?(DuckDB::InstanceCache)
+unless defined?(DuckDB::InstanceCache)This ensures the class is only defined if it doesn't already exist, preventing potential redefinition issues.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if defined?(DuckDB::InstanceCache) | |
| module DuckDB | |
| # The DuckDB::InstanceCache is necessary if a client/program (re)opens | |
| # multiple databases to the same file within the same statement. | |
| # | |
| # require 'duckdb' | |
| # cache = DuckDB::InstanceCache.new | |
| # db1 = cache.get(path: 'db.duckdb') | |
| # db2 = cache.get(path: 'db.duckdb') | |
| class InstanceCache | |
| def get(path: nil, config: nil) | |
| get_or_create(path, config) | |
| end | |
| end | |
| end | |
| end | |
| unless defined?(DuckDB::InstanceCache) | |
| module DuckDB | |
| # The DuckDB::InstanceCache is necessary if a client/program (re)opens | |
| # multiple databases to the same file within the same statement. | |
| # | |
| # require 'duckdb' | |
| # cache = DuckDB::InstanceCache.new | |
| # db1 = cache.get(path: 'db.duckdb') | |
| # db2 = cache.get(path: 'db.duckdb') | |
| class InstanceCache | |
| def get(path: nil, config: nil) | |
| get_or_create(path, config) | |
| end | |
| end | |
| end | |
| end |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
5-5: Clarify the new caching functionality in the changelog entry.The changelog entry currently reads:
- implement `DuckDB::InstanceCache` class.Since the PR focuses on adding the
DuckDB::InstanceCache#get_or_createmethod—which wraps the C-API functionduckdb_get_or_create_from_cache—it would be more informative to include this detail. Consider updating the entry to something like:- implement `DuckDB::InstanceCache` class. + implement `DuckDB::InstanceCache` class with `#get_or_create` method as a wrapper for `duckdb_get_or_create_from_cache`.This update helps users quickly understand the specific functionality added by this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (head, 1.2.2)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (3.1.6, 1.1.3)
- GitHub Check: test (head, 1.2.2)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (ucrt, 1.2.2)
- GitHub Check: test (mingw, 1.1.3)
- GitHub Check: test (3.3.6, 1.1.3)
- GitHub Check: test (mingw, 1.2.2)
- GitHub Check: test (ucrt, 1.1.3)
- GitHub Check: test (3.1.6, 1.1.3)
- GitHub Check: test (3.4.1, 1.2.2)
- GitHub Check: test (3.2.6, 1.1.1)
- GitHub Check: test (3.3.6, 1.2.2)
- GitHub Check: test (3.2.6, 1.2.2)
- GitHub Check: test (3.2.6, 1.1.3)
- GitHub Check: test (3.3.6, 1.1.1)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.1.6, 1.2.2)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/duckdb/instance_cache.rb (1)
3-26:⚠️ Potential issueIncorrect conditional check for module definition
The current implementation wraps the entire module definition in an
if defined?(DuckDB::InstanceCache)check. This is counterintuitive, as it means the class will only be defined if it's already defined elsewhere. Typically, this pattern is used to prevent redefinition, but the condition should be negated:-if defined?(DuckDB::InstanceCache) +unless defined?(DuckDB::InstanceCache)This ensures the class is only defined if it doesn't already exist, preventing potential redefinition issues.
🧹 Nitpick comments (3)
lib/duckdb/instance_cache.rb (3)
14-16: Inconsistent method documentation for return valueThe
:call-seq:documentation indicates the method returnsself, but according to the description on line 17, it should return aDuckDB::Databaseobject.- # instance_cache.get(path:, config:) -> self + # instance_cache.get(path:, config:) -> DuckDB::DatabaseEnsure documentation accurately reflects the actual return type.
20-22: Missing documentation for native method dependencyThe
getmethod delegates toget_or_create, which appears to be a native method implemented in the C extension but isn't defined in this Ruby file. Consider adding a comment to make this dependency clear:def get(path: nil, config: nil) + # Delegates to native method implemented in ext/duckdb/instance_cache.c get_or_create(path, config) endThis improves code maintainability by explicitly documenting that
get_or_createis a native method.
1-27: Implementation looks good with minor documentation improvements neededThe class implementation is clean and well-documented, with good usage examples. The class effectively serves its purpose of providing a cache for database instances, which is useful when multiple databases are opened to the same file.
Consider adding more details to the class documentation about:
- Thread safety considerations
- When to use this vs. directly creating database instances
- Any performance implications
This would provide more context for users of this class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ext/duckdb/instance_cache.c(3 hunks)lib/duckdb/instance_cache.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ext/duckdb/instance_cache.c
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: test (mswin, 1.1.1)
- GitHub Check: test (3.2.7, 1.1.3)
- GitHub Check: test (3.4.2, 1.1.3)
- GitHub Check: test (3.3.7, 1.1.1)
- GitHub Check: test (3.3.7, 1.1.3)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (head, 1.2.2)
- GitHub Check: test (3.3.7, 1.1.1)
- GitHub Check: test (head, 1.1.1)
- GitHub Check: test (3.2.7, 1.1.3)
- GitHub Check: test (head, 1.1.3)
- GitHub Check: test (3.3.7, 1.1.3)
- GitHub Check: test (3.1.6, 1.2.2)
- GitHub Check: test (3.3.7, 1.2.2)
- GitHub Check: test (3.4.2, 1.1.1)
- GitHub Check: test (3.4.2, 1.2.2)
- GitHub Check: test (3.2.7, 1.2.2)
- GitHub Check: test (3.1.6, 1.1.1)
- GitHub Check: test (3.4.2, 1.1.3)
- GitHub Check: test (3.2.7, 1.1.1)
- GitHub Check: test (3.1.6, 1.1.3)
1 participant
DuckDB::InstanceCache#get_or_createis wrapping C-APIduckdb_get_or_create_from_cache.Summary by CodeRabbit
get_or_createmethod under various scenarios to ensure reliability.