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.
PNixx
commented
1 week ago
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.