Skip to content

Conversation

@zaleslaw
Copy link
Collaborator

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.

Fixes #1365

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.
Copy link
Contributor

Copilot AI left a 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 createConnection methods for DuckDb and Sqlite to 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) {
Copy link

Copilot AI Aug 14, 2025

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.

Suggested change
if (dbConfig.readOnly) {
return connection.use { conn ->
var originalAutoCommit: Boolean? = null
try {
if (dbConfig.readOnly) {
originalAutoCommit = conn.autoCommit
Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@Jolanrensen Jolanrensen left a 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)

…dling Streamlined `createConnection` for SQLite using `SQLiteConfig` for read-only mode. Improved `isInMemoryDuckDb` logic for better URL handling. Updated dependencies and tests for consistency.
@zaleslaw zaleslaw merged commit 57726b8 into master Aug 15, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants