Encapsulate format logic within Statement and helper classes #162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@leboshi need resolve conflicts |
…rgument to `execute`.
…act ActiveRecord code.
…a request without appending a `FORMAT` clause.
…ce to indicate that they're internal/private.
* Restore support for table/database creation with request settings. * Move improved "DB::Exception" handling into ResponseProcessor.
leboshi
force-pushed
the
refactor/encapsulate-format-logic
branch
from
2b4c2dc to
7fb1a7f
Compare
|
Sorry that took so long! It's been a busy couple months. |
PNixx
requested changes
Nov 22, 2024
lib/active_record/connection_adapters/clickhouse/schema_statements.rb
Outdated
Show resolved
Hide resolved
lib/active_record/connection_adapters/clickhouse/schema_statements.rb
Outdated
Show resolved
Hide resolved
| module ConnectionAdapters | ||
| module Clickhouse | ||
| class Statement | ||
| class FormatManager |
There was a problem hiding this comment.
This incorrect logic for request:
SELECT * FROM table WHERE column LIKE 'insert into%'We have a method exec_insert which clearly indicates that this is an insert and what response format should be used.
lib/active_record/connection_adapters/clickhouse/statement/response_processor.rb
Outdated
Show resolved
Hide resolved
| end | ||
|
|
||
| def format_from_json_compact_each_row_with_names_and_types(body) | ||
| rows = body.split("\n").map { |row| parse_json_payload(row) } |
There was a problem hiding this comment.
body.each_line.map should be more efficient since it doesn't need to allocate an array before iterating over it to parse the JSON.
…erring within FormatManager
|
OK, all the above should be resolved! Thanks to both of you for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Handling different I/O formats added a lot of complexity to the codebase: whenever a specific format is needed or requested, that format has to be passed around to whatever handles the response so that it can be parsed correctly. Various methods around parsing responses according to different formats have been growing and collecting within the SchemaStatements module, where they don't really seem to belong.
process_responsealso has nestedcasestatements that are hard to grok.This PR extracts three helper classes:
FormatManagerto determine when to automatically apply aFORMATclause to a SQL statement,ResponseProcessorto parse the response according to the requested format and handle errors, andStatementto couple them together and maintain the format state.The first two are internal to Statement and have no reason to be used directly outside of it.
Breaking Changes
DEFAULT_RESPONSE_FORMATconstant into ClickhouseAdapter, since it's now used by helper classes instead of only within the SchemaStatements module.Deprecations
format:keyword toexecuteis deprecated (with no specific horizon yet) in favor of the newwith_response_formatwrapper method. The deprecation warning points users to the new method.executeanddo_executehad effectively identical signatures, sodo_executehas been deprecated.Next Steps
The
skip_format?check in the FormatManager really just includes checks that were already being run plus a few more we've found. They are not comprehensive. Ideally, we should go through the documentation to identify exactly which statements do or don't accept aFORMATclause.The chain of conditionals is also less than ideal. It's ugly but manageable for now. Whenever the complete list of inclusions/exclusions materializes, it may be better to apply a filter pattern to clean up the checks.