Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add settings final clause #292

Merged
merged 10 commits into from
Mar 14, 2024

Conversation

limonyellow
Copy link
Contributor

@limonyellow limonyellow commented Feb 9, 2024

Adding SETTINGS final = 1 in the end of the query.

This is a useful ClickHouse feature to apply FINAL to all tables in the query:

"Automatically applies FINAL modifier to all tables in a query, to tables where FINAL is applicable, including joined tables and tables in sub-queries, and distributed tables."
https://clickhouse.com/docs/en/operations/settings/settings#final

A known issue when having joined tables in query.
ClickHouse/ClickHouse#8655

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Ensure PR doesn't contain untouched code reformatting: spaces, etc.
  • Run flake8 and fix issues.
  • Run pytest no tests failed. See https://clickhouse-sqlalchemy.readthedocs.io/en/latest/development.html.

Tests fix:
fix(parsing): change the way to quote db_url to fix the test broken by sqlalchemy 2.0.25

@xzkostyan
Copy link
Owner

There is more convenient way to define settings with execution_options: https://clickhouse-sqlalchemy.readthedocs.io/en/latest/features.html#execution-options. This separates settings a little bit from query itself.

But this only is not supported for HTTP-driver.

For HTTP-driver you must add SETTINGS in query. In Native settings are passed in special binary block.

There will be conflict in native driver if this PR will be merged. ClickHouse server will get two section setting -- in binary form and in text form.

There are two options:

  1. we use .settings_final() only for HTTP and move these changes to HTTP-dialect (this covers only one setting final)
  2. we implement passing settings with execution_options for HTTP (this will cover all settings). You don't need to write every server setting in driver, just form SETTINGS clause with strings concatenation here clickhouse_sqlalchemy.drivers.http.connector.Cursor.execute.

I'd recommend to implement the second option. It will be more flexible.

@limonyellow
Copy link
Contributor Author

There is more convenient way to define settings with execution_options: https://clickhouse-sqlalchemy.readthedocs.io/en/latest/features.html#execution-options. This separates settings a little bit from query itself.

But this only is not supported for HTTP-driver.

For HTTP-driver you must add SETTINGS in query. In Native settings are passed in special binary block.

There will be conflict in native driver if this PR will be merged. ClickHouse server will get two section setting -- in binary form and in text form.

There are two options:

  1. we use .settings_final() only for HTTP and move these changes to HTTP-dialect (this covers only one setting final)
  2. we implement passing settings with execution_options for HTTP (this will cover all settings). You don't need to write every server setting in driver, just form SETTINGS clause with strings concatenation here clickhouse_sqlalchemy.drivers.http.connector.Cursor.execute.

I'd recommend to implement the second option. It will be more flexible.

Thanks for your insight. Let me know if this is what you meant.

@xzkostyan
Copy link
Owner

Yes, that's it.

@xzkostyan xzkostyan merged commit 3576913 into xzkostyan:master Mar 14, 2024
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants