-
Notifications
You must be signed in to change notification settings - Fork 358
Fix!: Avoid using rendered query when computing the data hash #5256
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
Changes from all commits
f0798ba
77f4197
353dbdd
07b0ec5
6062c94
7156542
988379c
26226d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| prepare_env, | ||
| serialize_env, | ||
| ) | ||
| from sqlmesh.utils.pydantic import PydanticModel, ValidationInfo, field_validator | ||
| from sqlmesh.utils.pydantic import PydanticModel, ValidationInfo, field_validator, get_dialect | ||
|
|
||
| if t.TYPE_CHECKING: | ||
| from sqlglot.dialects.dialect import DialectType | ||
|
|
@@ -616,11 +616,6 @@ def parse_strings_with_macro_refs(value: t.Any, dialect: DialectType) -> t.Any: | |
|
|
||
|
|
||
| expression_validator: t.Callable = field_validator( | ||
| "query", | ||
| "expressions_", | ||
| "pre_statements_", | ||
| "post_statements_", | ||
| "on_virtual_update_", | ||
| "unique_key", | ||
| mode="before", | ||
| check_fields=False, | ||
|
|
@@ -663,3 +658,65 @@ def parse_strings_with_macro_refs(value: t.Any, dialect: DialectType) -> t.Any: | |
| mode="before", | ||
| check_fields=False, | ||
| )(depends_on) | ||
|
|
||
|
|
||
| class ParsableSql(PydanticModel): | ||
| sql: str | ||
|
|
||
| _parsed: t.Optional[exp.Expression] = None | ||
| _parsed_dialect: t.Optional[str] = None | ||
|
|
||
| def parse(self, dialect: str) -> exp.Expression: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does the dialect ever change for a model outside of a test?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't, why? I'd rather implement this correctly in case circumstances change. |
||
| if self._parsed is None or self._parsed_dialect != dialect: | ||
| self._parsed = d.parse_one(self.sql, dialect=dialect) | ||
| self._parsed_dialect = dialect | ||
| return self._parsed | ||
|
|
||
| @classmethod | ||
| def from_parsed_expression( | ||
| cls, parsed_expression: exp.Expression, dialect: str, use_meta_sql: bool = False | ||
| ) -> ParsableSql: | ||
| sql = ( | ||
| parsed_expression.meta.get("sql") or parsed_expression.sql(dialect=dialect) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are the situation where we wouldn't want to use meta sql?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when I'm using a custom loader and do |
||
| if use_meta_sql | ||
| else parsed_expression.sql(dialect=dialect) | ||
| ) | ||
| result = cls(sql=sql) | ||
| result._parsed = parsed_expression | ||
| result._parsed_dialect = dialect | ||
| return result | ||
|
|
||
| @classmethod | ||
| def validator(cls) -> classmethod: | ||
| def _validate_parsable_sql( | ||
| v: t.Any, info: ValidationInfo | ||
| ) -> t.Optional[t.Union[ParsableSql, t.List[ParsableSql]]]: | ||
| if v is None: | ||
| return v | ||
| if isinstance(v, str): | ||
| return ParsableSql(sql=v) | ||
| if isinstance(v, exp.Expression): | ||
izeigerman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return ParsableSql.from_parsed_expression( | ||
| v, get_dialect(info.data), use_meta_sql=False | ||
| ) | ||
| if isinstance(v, list): | ||
| dialect = get_dialect(info.data) | ||
| return [ | ||
| ParsableSql(sql=s) | ||
| if isinstance(s, str) | ||
| else ParsableSql.from_parsed_expression(s, dialect, use_meta_sql=False) | ||
| if isinstance(s, exp.Expression) | ||
| else ParsableSql.parse_obj(s) | ||
| for s in v | ||
| ] | ||
| return ParsableSql.parse_obj(v) | ||
|
|
||
| return field_validator( | ||
| "query_", | ||
| "expressions_", | ||
| "pre_statements_", | ||
| "post_statements_", | ||
| "on_virtual_update_", | ||
| mode="before", | ||
| check_fields=False, | ||
| )(_validate_parsable_sql) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this called multiple times? should we cache this somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, you use a class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's cached inside
ParsableQuery