support execute_batch #2
Conversation
Rails is using `execute_batch` when truncating tables and loading fixtures. https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L232 https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L515 But since this gem is changing signature of `raw_execute`, then it does not work out of the box and needs to call `execute` instead. https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L616-L620
WalkthroughIntroduces a new public method execute_batch(statements, name = nil, **kwargs) to iterate over an array of SQL statements and call execute for each, delegating to the existing execution path without altering error handling or other control flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant SchemaStatements as Clickhouse::SchemaStatements
participant Adapter as execute()
Caller->>SchemaStatements: execute_batch(statements, name, **kwargs)
loop for each statement in statements
SchemaStatements->>Adapter: execute(statement, name, **kwargs)
Adapter-->>SchemaStatements: result
end
SchemaStatements-->>Caller: nil or last result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| def execute_batch(statements, name = nil, **kwargs) | ||
| statements.each do |statement| | ||
| execute(statement, name, **kwargs) | ||
| end | ||
| end |
There was a problem hiding this comment.
Restore fallback for single-statement batches
The upstream ActiveRecord::ConnectionAdapters::DatabaseStatements#execute_batch accepts either an enumerable of statements or a single statement and only iterates when the argument responds to to_ary. By overriding it with statements.each, calling execute_batch("TRUNCATE ...") will now raise NoMethodError (Strings and many other statement objects don’t implement each). Please keep parity with the abstract adapter and fall back to a single execute call when statements isn’t array-like.
def execute_batch(statements, name = nil, **kwargs)
- statements.each do |statement|
- execute(statement, name, **kwargs)
- end
+ if statements.respond_to?(:to_ary)
+ statements.to_ary.each { |statement| execute(statement, name, **kwargs) }
+ else
+ execute(statements, name, **kwargs)
+ end
end📝 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.
| def execute_batch(statements, name = nil, **kwargs) | |
| statements.each do |statement| | |
| execute(statement, name, **kwargs) | |
| end | |
| end | |
| def execute_batch(statements, name = nil, **kwargs) | |
| if statements.respond_to?(:to_ary) | |
| statements.to_ary.each { |statement| execute(statement, name, **kwargs) } | |
| else | |
| execute(statements, name, **kwargs) | |
| end | |
| end |
🤖 Prompt for AI Agents
In lib/active_record/connection_adapters/clickhouse/schema_statements.rb around
lines 50 to 54, the current implementation always calls statements.each which
breaks when a single statement string is passed; change it to detect array-like
input (e.g. if statements.respond_to?(:to_ary) or similar) and iterate only in
that case, otherwise call execute(statements, name, **kwargs) once so behavior
matches ActiveRecord's fallback for single-statement batches.
1 participant
Rails is using
execute_batchwhen truncating tables and loading fixtures.https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L232
https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L515
But since this gem is changing signature of
raw_execute, then it does not work out of the box and needs to callexecuteinstead.https://github.com/rails/rails/blob/bab99f8b51dfbff4d9e161b8f23c0dbf5d3b9314/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L616-L620
PR upstream opened PNixx#216
Summary by CodeRabbit