fix(dialect): make extend_sqlglot idempotent - #5921
Conversation
StuffbyYuki
left a comment
There was a problem hiding this comment.
@Sanjays2402 Thanks for the PR! Here are a few things (not blockers):
- Also guard
generator.UNWRAPPED_INTERVAL_VALUES(still appends every call; TRANSFORMS / WITH_SEPARATED_COMMENTS already have membership guards — called out in #5908). - Optional assert that a second call doesn’t grow
UNWRAPPED_INTERVAL_VALUES(or that__parse_typesstill points at sqlglot’s original).
Merge ready once ci tests pass
|
Thanks @StuffbyYuki — pushed 5f2c7b9. Added a On the 3.9 style job: the 12 failures are all |
_override saved whatever was currently installed under _<name>, so a second extend_sqlglot() call saved the override itself as the original. The wrapper then called itself and every subsequent type/CAST parse raised RecursionError, with sqlglot's original method lost. _override now returns early when the override is already installed. Adds tests/core/test_dialect.py::test_extend_sqlglot_is_idempotent, which fails with RecursionError without the fix. Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>
5f2c7b9 to
132f16b
Compare
|
@Sanjays2402 Can you add DCO checks? |
Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>
132f16b to
fb992b7
Compare
|
Done — signed off the second commit and force-pushed, head is now fb992b7. Amended with |
Closes #5908
Description
_overridesaved whatever was currently installed under_<name>, so a secondextend_sqlglot()call saved the override itself as the "original". The wrapper then called itself and every subsequent type/CAST parse raisedRecursionError, with sqlglot's original method lost. This is reachable without calling it twice on purpose:extend_sqlglot()is the first statement ofsqlmesh/__init__.py, so if anything later in that import cascade raises, the package is evicted fromsys.moduleswhile the patched globals survive, and the retry re-applies the patch._overridenow returns early when the override is already installed.Test Plan
Added
tests/core/test_dialect.py::test_extend_sqlglot_is_idempotent, which fails withRecursionErrorwithout the fix and passes with it.tests/core/test_dialect.pyis green (160 passed).Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO