-
Notifications
You must be signed in to change notification settings - Fork 76
Add support for read-only mode in DuckDB and SQLite #1383
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
Conversation
This commit introduces `createConnection` methods for `DuckDb` and `Sqlite` to handle read-only mode during connection creation. It also updates test configurations and ensures proper handling of in-memory databases.
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.
Pull Request Overview
This PR adds support for read-only database connections in DuckDB and SQLite by implementing database-specific connection creation logic. The main changes address the limitation where some databases require read-only mode to be configured during connection establishment rather than after.
- Implements
createConnectionmethods forDuckDbandSqliteto handle read-only mode appropriately - Updates test configurations to use temporary files instead of in-memory databases for proper read-only testing
- Refactors connection handling logic to use database-specific connection creation
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/DbType.kt | Adds base createConnection method with default read-only handling |
| dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/DuckDb.kt | Implements DuckDB-specific connection creation with read-only property support and in-memory database validation |
| dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/Sqlite.kt | Implements SQLite-specific connection creation using reflection for SQLiteConfig |
| dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/readJdbc.kt | Updates connection handling to use database-specific connection creation |
| dataframe-jdbc/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/sqliteTest.kt | Changes from in-memory to temporary file database for read-only testing |
| dataframe-jdbc/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/local/duckDbTest.kt | Updates read-only tests to handle in-memory database limitations |
| dataframe-jdbc/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/h2/h2Test.kt | Minor H2 connection string update |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| return connection.use { conn -> | ||
| try { | ||
| if (dbConfig.readOnly) { |
Copilot
AI
Aug 14, 2025
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.
The connection properties (autoCommit) are being modified but not restored after use. Consider saving the original state and restoring it in the finally block to ensure proper cleanup in pooled connection environments.
| if (dbConfig.readOnly) { | |
| return connection.use { conn -> | |
| var originalAutoCommit: Boolean? = null | |
| try { | |
| if (dbConfig.readOnly) { | |
| originalAutoCommit = conn.autoCommit |
dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/Sqlite.kt
Outdated
Show resolved
Hide resolved
Jolanrensen
left a comment
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.
Some small things only, good fix :)
(requires an apiDump call)
dataframe-jdbc/src/test/kotlin/org/jetbrains/kotlinx/dataframe/io/local/duckDbTest.kt
Outdated
Show resolved
Hide resolved
dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/DbType.kt
Outdated
Show resolved
Hide resolved
dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/Sqlite.kt
Outdated
Show resolved
Hide resolved
dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/DuckDb.kt
Show resolved
Hide resolved
dataframe-jdbc/src/main/kotlin/org/jetbrains/kotlinx/dataframe/io/db/DuckDb.kt
Outdated
Show resolved
Hide resolved
…dling Streamlined `createConnection` for SQLite using `SQLiteConfig` for read-only mode. Improved `isInMemoryDuckDb` logic for better URL handling. Updated dependencies and tests for consistency.
This commit introduces
createConnectionmethods forDuckDbandSqliteto handle read-only mode during connection creation. It also updates test configurations and ensures proper handling of in-memory databases.Fixes #1365