Skip to content

refactor: make hash and eq non recursive#5966

Merged
tobymao merged 1 commit intomainfrom
toby/eq
Sep 26, 2025
Merged

refactor: make hash and eq non recursive#5966
tobymao merged 1 commit intomainfrom
toby/eq

Conversation

@tobymao
Copy link
Owner

@tobymao tobymao commented Sep 25, 2025

No description provided.

@georgesittas
Copy link
Collaborator

georgesittas commented Sep 25, 2025

Was this benchmarked? I've two branches where I refactored the implementations as well (2 different ways), but they were slower compared to our current implementation and we decided to not use them.

Will review tomorrow, haven't checked whether/how they're different from the approach here.

@tobymao
Copy link
Owner Author

tobymao commented Sep 25, 2025

Yea, it could be faster. It also leverages caching in better ways. So it should make the optimizer way faster too.

Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Nice work. I'd add this test as well to demonstrate that the PR solves the issue:

    def test_hash_large_asts(self):
        # Ensures we can actually compare large ASTs and not fail due to exceeding recursion limit
        ast = parse_one("SELECT 1 UNION ALL " * 500 + "SELECT 1")
        assert isinstance(hash(ast), int)

@tobymao tobymao merged commit d425ba2 into main Sep 26, 2025
6 checks passed
@tobymao tobymao deleted the toby/eq branch September 26, 2025 22:15
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