Conversation
3fd7c5d to
a90a4bd
Compare
Signed-off-by: vaggelisd <daniasevangelos@gmail.com>
a90a4bd to
8df9c29
Compare
|
The DuckDB test failure is preexisting |
| # Clear meta["sql"] (set by our parser extension) so the pydantic encoder | ||
| # uses dialect-aware rendering: e.sql(dialect=meta["dialect"]). Without this, | ||
| # the raw SQL text takes priority, which can be wrong for dialect-normalized | ||
| # types (e.g., default "TIMESTAMP" should render as "DATETIME" in BigQuery). | ||
| data_type.meta.pop("sql", None) |
There was a problem hiding this comment.
Is there a test for this? Why did this work before the bump? Do we risk messing up the serialized version of this field by popping sql?
There was a problem hiding this comment.
I think it was because of this commit tobymao/sqlglot#7092.
On v28, we'd:
- Parse the string text into
exp.DataTypewithparse_one(settingmeta["sql"]in the process) - Reconstruct a fresh object again before returning, effectively discarding
meta
On v30 we only do (1) now after the optimization PR.
We should have the same serialization and data hashes, otherwise e.g we'd notice BigQuery parsing "TIMESTAMP" into exp.DType.DATETIME but roundtripping it to "TIMESTAMP" again due to meta taking priority as raw text verbatim.
There was a problem hiding this comment.
Ok, so the current behavior is coincidentally consistent with what we used to do before the migration, because of the buggy AST node reconstruction in v28?
sqlmesh/lsp/main.py
Outdated
| source=source, | ||
| target=target, | ||
| on=exp.condition(on) if on else None, | ||
| on=t.cast(exp.Condition, exp.condition(on)) if on else None, |
There was a problem hiding this comment.
Why do we need a cast here?
There was a problem hiding this comment.
Ah, on v30 that was widened to exp.Expr, I'll widen the type hints for all the call chains
| try: | ||
| parent_module = sys.modules.get(parent) or importlib.import_module(parent) | ||
| if getattr(parent_module, name, None) is obj: | ||
| return parent |
There was a problem hiding this comment.
Doesn't this only work for 1 parent right now? For example, if the new module became sqlglot.expressions.foo.bar, would you stop at foo?
There was a problem hiding this comment.
This is under a for loop walking up to the root module, this should work I believe
There was a problem hiding this comment.
I meant that if foo re-exports bar in the above example, don't we stop at foo? Which means that we'll still see a diff in the serialized import due to a different path.
Signed-off-by: vaggelisd <daniasevangelos@gmail.com>
45dbbbc to
264240e
Compare
Summary of Changes
pyproject.tomlsqlglot[rs]~=28.10.1→sqlglot~=30.0.1[rs]extra removed (replaced bysqlglotc)sqlmesh/,tests/,web/exp.Expression→exp.Exprin all type hints, isinstance checks, and type casts. Class definitions remain asexp.Expressionsubclassesexp.Expras the new base class;Expressionis now a subclass. Functions likeexp.alias_(),exp.func(),parse_one()returnExpr, causing mypy errors when assigned toExpression-typed variables. Custom expression classes must still inheritExpressionsinceExprdoesn't support constructor argumentssqlmesh/core/schema_diff.py,tests/core/integration/utils.pyexp.DataType.Type→exp.DTypein type annotationsDataType.Typeenum is no longer recognized as a valid type by mypy;exp.DTypeis the v30 aliassqlmesh/core/dialect.pyAthenaTrinoParser, updatedself.expression()callsexpression()now takes pre-constructed objectssqlmesh/utils/jinja.pyExpressionfromsqlglot.expressionsinstead ofsqlglotsqlglot.expressionsis now a package; top-level re-export changedsqlmesh/core/model/kind.pymeta["sql"]in_time_data_type_validatorDataType.buildnow preserves the parsed expression directly. Our_parse_typesextension setsmeta["sql"], which the pydantic encoder prioritizes over dialect-aware renderingsqlmesh/core/model/meta.pyINTno longer auto-maps toBIGINTduringDataType.build; roundtripping through dialect SQL restores canonical form for stable data hashessqlmesh/utils/metaprogramming.py_resolve_import_module()to walk module hierarchy for re-exportsto_table.__module__is nowsqlglot.expressions.buildersbut it's re-exported fromsqlglot.expressions; generated import statements must use the public modulesqlmesh/core/context_diff.pysqlglotctoIGNORED_PACKAGESsqlglotc(C extension); dependency detection was picking it up as a user requirementtests/core/test_macros.pyARRAY_GENERATE_RANGEarithmetic(DATEDIFF(...) + 1 - 1) + 1toDATEDIFF(...) + 1tests/core/test_config.pyColumnclass now reports__module__assqlglot.expressions.coreinstead ofsqlglot.expressions