clickhouse-activerecord icon indicating copy to clipboard operation
clickhouse-activerecord copied to clipboard

Encapsulate format logic within Statement and helper classes

Open leboshi opened this issue 1 year ago • 1 comments

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_response also has nested case statements that are hard to grok.

This PR extracts three helper classes:

  • FormatManager to determine when to automatically apply a FORMAT clause to a SQL statement,
  • ResponseProcessor to parse the response according to the requested format and handle errors, and
  • Statement to 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

  • Move DEFAULT_RESPONSE_FORMAT constant into ClickhouseAdapter, since it's now used by helper classes instead of only within the SchemaStatements module.

Deprecations

  • Passing format: keyword to execute is deprecated (with no specific horizon yet) in favor of the new with_response_format wrapper method. The deprecation warning points users to the new method.
  • execute and do_execute had effectively identical signatures, so do_execute has 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 a FORMAT clause.

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.

leboshi avatar Sep 06 '24 09:09 leboshi

@leboshi need resolve conflicts

PNixx avatar Oct 23 '24 10:10 PNixx

Sorry that took so long! It's been a busy couple months.

leboshi avatar Nov 22 '24 00:11 leboshi

OK, all the above should be resolved! Thanks to both of you for the review!

leboshi avatar Dec 06 '24 20:12 leboshi

🎉 We rebased, got tests passing, and have this branch working against our code, except for one other PR we're about to submit. The last requested change about exec_insert has also been made. Should be good to go!

leboshi avatar Jul 24 '25 00:07 leboshi