Feature/Update core DuckDB adapter implementation #5

eddietejeda · 2025-06-22T15:11:24Z

Update database_statements.rb transaction support and query execution, default to using @raw_connection, and add support for binding params
Add schema_statements.rb DDL operations (CREATE/ALTER/DROP table support)
BREAKING: Remove legacy Railtie approach and use modern ActiveRecord::ConnectionAdapters.register approach for registering gem
Add YARD documentation to make it clear which files were implementing necessary methods
Default to writing DuckDB file to disk
Validated lifecycle of creating and deploying databases and tested bundle exec rails db:drop db:create db:migrate db:seed db:reset
Use DuckDB's information_schema for accessing meta data
Default primary ids as bigints

- Update database_statements.rb transaction support and query execution, default to using @raw_connection, and add support for binding params - Add schema_statements.rb DDL operations (CREATE/ALTER/DROP table support) - BREAKING: Remove legacy Railtie approach and use modern ActiveRecord::ConnectionAdapters.register approach for registering gem - Add YARD documentation to make it clear which files were implementing necessary methods - Default to writing DuckDB file to disk - Validated lifecycle of creating and deploying databases and tested bundle exec rails db:drop db:create db:migrate db:seed db:reset - Use DuckDB's information_schema for accessing meta data - Default primary ids as bigints

kou · 2025-06-23T02:21:19Z

lib/active_record/connection_adapters/duckdb/database_statements.rb

+        # @param [Boolean] materialize_transactions Whether to materialize transactions
+        # @return [Object] Query result
+        def internal_execute(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false, materialize_transactions: true, &block)
+          raw_execute(sql, name, binds, prepare: prepare, async: async, allow_retry: allow_retry, materialize_transactions: materialize_transactions, &block)


It seems that raw_execute is used only in this method.
How about inlining the raw_execute implementation here and remove raw_execute?

BTW, can we use the default implementations of internal_execute and raw_execute in Active Record? If we can use them, we can reduce maintenance cost.

kou · 2025-06-23T02:23:17Z

lib/active_record/connection_adapters/duckdb/database_statements.rb

+          result = internal_execute(sql, name)
+          result


Suggested change

result = internal_execute(sql, name)

result

internal_execute(sql, name)

kou · 2025-06-23T02:26:46Z

lib/active_record/connection_adapters/duckdb/database_statements.rb

+        def exec_insert(sql, name = nil, binds = [], pk = nil, sequence_name = nil, returning: nil)
+          if pk && supports_insert_returning?
+            # Use INSERT...RETURNING to get the inserted ID
+            returning_sql = sql.sub(/\bINSERT\b/i, "INSERT").concat(" RETURNING #{quote_column_name(pk)}")


It seems that we don't need the sub here:

Suggested change

returning_sql = sql.sub(/\bINSERT\b/i, "INSERT").concat(" RETURNING #{quote_column_name(pk)}")

returning_sql = "#{sql} RETURNING #{quote_column_name(pk)}"

BTW, do we need to override exec_insert? It seems that we can use the default sql_for_insert and exec_insert implementations in Active Record.

kou · 2025-06-23T02:30:56Z

lib/active_record/connection_adapters/duckdb/database_statements.rb

+        # @note convert Arel to SQL string
+        # @param [Object] arel Arel object or SQL string
+        # @param [Array] binds Bind parameters (unused)
+        # @return [String] SQL string
+        def to_sql(arel, binds = [])
+          if arel.respond_to?(:to_sql)
+            arel.to_sql
+          else
+            arel.to_s
+          end
+        end


Do we need this? Can we use the default implementation in Active Record?

kou · 2025-06-23T02:32:00Z

lib/active_record/connection_adapters/duckdb/database_statements.rb

+          # Our simplified implementation for basic cases
+          if arel_or_sql_string.respond_to?(:ast)
+            # For Arel objects, visit the AST to get SQL and collect binds
+            visitor = arel_visitor
+            collector = Arel::Collectors::SQLString.new
+            visitor.accept(arel_or_sql_string.ast, collector)
+            sql = collector.value
+
+            # Extract binds from the visitor if it collected them
+            visitor_binds = if visitor.respond_to?(:binds)
+              visitor.binds
+            else
+              []
+            end
+
+            result = [sql, binds + visitor_binds]
+            # Add any additional args back to maintain signature compatibility
+            args.each { |arg| result << arg }
+            result
+          elsif arel_or_sql_string.respond_to?(:to_sql)
+            # For objects with to_sql method, use it directly
+            result = [arel_or_sql_string.to_sql, binds]
+            args.each { |arg| result << arg }
+            result
+          else
+            # For plain strings, return as-is
+            result = [arel_or_sql_string.to_s, binds]
+            args.each { |arg| result << arg }
+            result
+          end


Why do we need the custom implementation for this? If we can use the default implementation, we can reduce maintenance cost.

kou · 2025-06-23T03:00:47Z

lib/active_record/connection_adapters/duckdb_adapter.rb

+      # @override
+      # @note Implements AbstractAdapter interface method
+      # @return [String] The adapter name
+      def adapter_name # :nodoc:
+        "DuckDB"
      end


We can use the default implementation.

Suggested change

# @override

# @note Implements AbstractAdapter interface method

# @return [String] The adapter name

def adapter_name # :nodoc:

"DuckDB"

end

kou · 2025-06-23T03:08:52Z

lib/active_record/connection_adapters/duckdb_adapter.rb

+      # @override
+      # @note Implements AbstractAdapter interface method
+      # @param [String] table_name Name of the table
+      # @return [Array<ActiveRecord::ConnectionAdapters::Column>] Array of column objects
+      def columns(table_name) # :nodoc:
+        column_definitions(table_name).map do |field|
+          new_column_from_field(table_name, field)
+        end
+      end


Can we use the default implementation?

kou · 2025-06-23T03:10:50Z

lib/active_record/connection_adapters/duckdb_adapter.rb

+        # @param [Array] binds Bind parameters
+        # @param [Boolean] prepare Whether to prepare statement
+        # @param [Boolean] async Whether to execute asynchronously
+        # @return [Object] Query result
        def execute_and_clear(sql, name, binds, prepare: false, async: false)


It seems that this isn't used now. Can we remove this?

kou · 2025-06-23T03:11:30Z

lib/active_record/connection_adapters/duckdb_adapter.rb

-            each_hash(result)
-          end
-        end
+                    # @note used by columns() method


Suggested change

# @note used by columns() method

# @note used by columns() method

kou · 2025-06-23T03:12:05Z

lib/active_record/connection_adapters/duckdb_adapter.rb

+          ActiveRecord::Relation::QueryAttribute.new("table_name", table_name, ActiveRecord::Type::String.new)
+        ]
+
+        result = internal_exec_query(sql, "SCHEMA", binds)


Can we use query instead of internal_exec_query here?

Copilot

Pull Request Overview

This PR introduces major updates to the core DuckDB adapter implementation for ActiveRecord, adding support for binding parameters, DDL operations, and improved connection handling while removing legacy Railtie support.

Update version information and remove deprecated Railtie registration
Add new registration via ActiveRecord::ConnectionAdapters.register and include enhanced methods for connecting, querying, and schema operations
Include comprehensive YARD documentation and enhanced error handling for file-based databases

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
lib/activerecord_duckdb_adapter/version.rb	Version updated from 0.1.0 to 0.2.0
lib/activerecord_duckdb_adapter.rb	Legacy Railtie removed
lib/activerecord-duckdb-adapter.rb	New adapter registration mechanism introduced
lib/active_record/connection_adapters/duckdb_adapter.rb	Core adapter updated for connection, quoting, and query execution
lib/active_record/connection_adapters/duckdb/schema_statements.rb	Added DDL operations and table creation enhancements
lib/active_record/connection_adapters/duckdb/database_statements.rb	Expanded query execution with parameter binding and detailed explanation support

Comments suppressed due to low confidence (1)

lib/active_record/connection_adapters/duckdb_adapter.rb:122

[nitpick] The 'reconnect' method currently only returns the existing connection without re-establishing it; if this behavior is intentional, consider renaming it to reflect that or adding reconnection logic for clarity.

      def reconnect

Copilot · 2025-06-23T03:13:12Z

lib/active_record/connection_adapters/duckdb_adapter.rb

+              rescue SystemCallError
+                raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)


Consider capturing and including the original SystemCallError details when directory creation fails, so that the raised ActiveRecord::NoDatabaseError provides more context for debugging.

Suggested change

rescue SystemCallError

raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)

rescue SystemCallError => e

raise ActiveRecord::NoDatabaseError.new(

"Failed to create directory '#{dirname}': #{e.message}",

connection_pool: @pool,

cause: e

)

Copilot · 2025-06-23T03:13:12Z

lib/active_record/connection_adapters/duckdb/database_statements.rb

+        # @param [Array] binds Bind parameters
+        # @param [Array] args Additional arguments
+        # @return [Array] Array containing SQL string and bind parameters
+        def to_sql_and_binds(arel_or_sql_string, binds = [], *args)


[nitpick] Consider refactoring 'to_sql_and_binds' to reduce duplication between handling Arel objects and plain SQL strings, which may improve clarity and maintainability.

eddietejeda · 2025-07-06T04:30:17Z

FYI: I needed to step away from this, but updates are coming later this month.

eddietejeda changed the title ~~Update core DuckDB adapter implementation~~ Feature / Update core DuckDB adapter implementation Jun 23, 2025

eddietejeda changed the title ~~Feature / Update core DuckDB adapter implementation~~ Feature/Update core DuckDB adapter implementation Jun 23, 2025

kou reviewed Jun 23, 2025

View reviewed changes

kou requested a review from Copilot June 23, 2025 03:12

Copilot AI reviewed Jun 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/Update core DuckDB adapter implementation #5

Feature/Update core DuckDB adapter implementation #5

Uh oh!

eddietejeda commented Jun 22, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

kou Jun 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 23, 2025

Uh oh!

Copilot AI Jun 23, 2025

Uh oh!

eddietejeda commented Jul 6, 2025

Uh oh!

Uh oh!

	result = internal_execute(sql, name)
	result
	internal_execute(sql, name)

	returning_sql = sql.sub(/\bINSERT\b/i, "INSERT").concat(" RETURNING #{quote_column_name(pk)}")
	returning_sql = "#{sql} RETURNING #{quote_column_name(pk)}"

	# @note used by columns() method
	# @note used by columns() method

		rescue SystemCallError
		raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)

-              rescue SystemCallError
-                raise ActiveRecord::NoDatabaseError.new(connection_pool: @pool)
+              rescue SystemCallError => e
+                raise ActiveRecord::NoDatabaseError.new(
+                  "Failed to create directory '#{dirname}': #{e.message}",
+                  connection_pool: @pool,
+                  cause: e
+                )

Feature/Update core DuckDB adapter implementation #5

Are you sure you want to change the base?

Feature/Update core DuckDB adapter implementation #5

Uh oh!

Conversation

eddietejeda commented Jun 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

eddietejeda commented Jul 6, 2025

Uh oh!

Uh oh!