Skip to content

Feat: allow overriding the dialects' normalization strategies#2779

Merged
georgesittas merged 8 commits intomainfrom
jo/override_normalization_strategy
Jun 19, 2024
Merged

Feat: allow overriding the dialects' normalization strategies#2779
georgesittas merged 8 commits intomainfrom
jo/override_normalization_strategy

Conversation

@georgesittas
Copy link
Contributor

No description provided.

@georgesittas georgesittas requested a review from tobymao June 17, 2024 10:40

The default model kind is `VIEW` unless overridden with the `kind` key. For more information on model kinds, refer to [model concepts page](../concepts/models/model_kinds.md).

##### Identifier resolution
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is overkill, but since there has been some discussion in Slack, I thought I'd do at least a first pass and then we could chat about it.

cc @tobymao @treysp

@georgesittas georgesittas requested a review from a team June 17, 2024 17:58
# applied to the DuckDB dialect globally
if "normalization_strategy" in str(self.config.dialect):
dialect = Dialect.get_or_raise(self.config.dialect)
type(dialect).NORMALIZATION_STRATEGY = dialect.normalization_strategy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I can't have different normalization strategies for the same dialect? Furthermore it can't be overridden on per model basis? Should this be a top-level config then instead of being a part of a dialect? Feels a little misleading otherwise.

Copy link
Contributor Author

@georgesittas georgesittas Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I can't have different normalization strategies for the same dialect?

That's right - but it seems reasonable to me, because you can only have a single default strategy for a given dialect. This simply makes the default_dialect override that default globally for said dialect, but it can be overridden on a model basis, so if you do dialect duckdb,normalization_strategy=... in the model meta, then I'm pretty sure that should take precedence over the class constant set here.

Copy link
Contributor Author

@georgesittas georgesittas Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I can't have different normalization strategies for the same dialect?

Ah, I guess you were asking if you can override that strategy per model? If so, my answer may seem a bit contradictory. You can have different normalization strategies for the same dialect, e.g. if you override it in a model.

EDIT: clarified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i'm confused, can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • You can set dialect: foo,normalization_strategy=... at the model defaults config level. This will monkey patch its NORMALIZATION_STRATEGY class constant, so it applies globally.
  • You can override this behavior per model by doing e.g. dialect foo,normalization_strategy=...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused though. Wouldn't setting it in a model still override the strategy globally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would it work if you set the model config to normarizaltion=x. would that get propogated to the engine adapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't setting it in a model still override the strategy globally?

Nope, because it gets stored as an attributed in the dialect and we ignore the NORMALIZATION_STRATEGY constant (see constructor). So there's no global overriding.

how would it work if you set the model config to normarizaltion=x. would that get propogated to the engine adapter?

If you do this at the model default config, then the dialect will be patched => the engine adapter's dialect will also have its strategy changed. This was the main motivation behind the patching.

If we didn't patch it, then setting normalization=.. in the project config would override the strategy of the dialect stored in the config, but not the one stored in the engine adapter, because we don't really pass the config's dialect to the adapters.

@georgesittas georgesittas merged commit 99c3384 into main Jun 19, 2024
@georgesittas georgesittas deleted the jo/override_normalization_strategy branch June 19, 2024 16:37
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.

3 participants