Skip to content
This repository was archived by the owner on May 17, 2024. It is now read-only.

Simplify: Squash the MD5 & NormalizerValue & OptimizerHints mixins into the base dialect #754

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Oct 25, 2023

Little improvements step by step:

Following the removal of unused mixins in #753, remove the last ones that are still used: OptimizerHints, MD5 & NormalizerValue.

Simply squash their methods into the base dialect class. The MD5 & Normalizer are present in ALL our databases, so there is no change required from the real dialect classes anyway.

The OptimizerHints mixin is present only in MS SQL, MySQL, and Oracle. But it was using the undeclared methods where called, so it was buggy anyway (if it was used at all). Now, we will add optimizer hints in all databases, though most of them will ignore this comment.

In the end, we get a plain & simple hierarchy of dialects: BaseDialect -> specific WhateverDialect — with no multiple inheritance and unclear source of methods.


If that helps, here is the matrix of databases & their mixins that they inherit from:

NV AbstractMixin_NormalizeValue
MD AbstractMixin_MD5
SC AbstractMixin_Schema                 Many, but no default behaviour; UNUSED? except in 2 tests
RS AbstractMixin_RandomSample           Only DuckDB? no default behaviour; UNUSED?
TT AbstractMixin_TimeTravel             Only BigQUery & Snowflake, no default behaviour; UNUSED?
OH AbstractMixin_OptimizerHints         Only MS SQL, MySQL, Oracle, no default behaviour
rSC Mixin_Schema                        UNUSED!
rRS Mixin_RandomSample                  UNUSED!
rOH Mixin_OptimizerHints


Dialect👉   AbstractMixin_…   Mixin_…
Db👇        NV MD SC RS TT OH rSC rRS rOH

bigquery    +  +  +  -  +  -   -   -   -
clickhouse  +  +  -  -  -  -   -   -   -
databricks  +  +  -  -  -  -   -   -   -
duckdb      +  +  +  +  -  -   +   -   -
mssql       +  +  +  -  -  +   +   -   +
mysql       +  +  +  -  -  +   +   -   +
oracle      +  +  +  -  -  +   -   -   +
postgresql  +  +  +  -  -  -   +   -   -
presto      +  +  +  -  -  -   +   -   -
snowflake   +  +  +  -  +  -   -   -   -
vertica     +  +  +  -  -  -   -   -   -

redshift    +  +  +  -  -  -   +   -   -  == postgres
trino       +  +  +  -  -  -   +   -   -  == presto

ALL of them +  +  -  -  -  -   -   -!  -

@nolar nolar requested review from dlawin and vvkh October 25, 2023 15:40
…alects (even when not used)

It is currently defined and used only in MS SQL, MySQL, and Oracle. Other databases simply do not support this method. It will not damage the query if we add the SQL comment there if the database does not support it.
@nolar nolar force-pushed the squash-abstract-mixins branch from 5656512 to 31475f9 Compare October 25, 2023 16:37
@nolar nolar marked this pull request as ready for review October 25, 2023 16:37
Sergey Vasilyev added 2 commits October 25, 2023 18:48
The MD5 & normalizing methods are implemented in every supported database with 100% coverage. We have no databases that do not implement these methods. As such, they can be simply moved to the base dialect class to ensure the 100% coverage in the future. No changes are required from the specific dialect classes.
@nolar nolar force-pushed the squash-abstract-mixins branch from 31475f9 to 9aa6e55 Compare October 25, 2023 16:48
@dlawin dlawin merged commit 4a21935 into master Oct 25, 2023
@dlawin dlawin deleted the squash-abstract-mixins branch October 25, 2023 18:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants