Skip to content
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

Feat: optionally wrap connectors when using builders #4369

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

georgesittas
Copy link
Collaborator

@georgesittas georgesittas commented Nov 11, 2024

Closes #4367
Closes #4362

Now this works:

>>> import sqlglot
>>> import sqlglot.expressions as expressions
>>> from sqlglot.expressions import column
>>>
>>> is_equal_list = ['a'] * 500
>>> is_equal = expressions.false()
>>>
>>>
>>> for value in is_equal_list:
...     is_equal = is_equal.or_(column("a_column").eq(value), wrap=False)
...
>>> is_equal.sql()[:100]
"FALSE OR a_column = 'a' OR a_column = 'a' OR a_column = 'a' OR a_column = 'a' OR a_column = 'a' OR a"

I didn't add a timing test because they're flaky. I think we should refactor the existing ones at some point too.

Copy link
Collaborator

@VaggelisD VaggelisD left a comment

Choose a reason for hiding this comment

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

RE: Iterative generation logic

Could we solve this issue by unwrapping exp.Paren during iteration instead of traversing downwards? This could solve both issues at once, unless the API addition here is nice to have anyways.

@georgesittas
Copy link
Collaborator Author

RE: Iterative generation logic

Could we solve this issue by unwrapping exp.Paren during iteration instead of traversing downwards? This could solve both issues at once, unless the API addition here is nice to have anyways.

Sure, wanna take a stab at it? I think conditionally wrapping in the API makes sense, but improving Generator.binary is also a good idea.

@VaggelisD
Copy link
Collaborator

Sure, wanna take a stab at it? I think conditionally wrapping in the API makes sense, but improving Generator.binary is also a good idea.

Sounds good, but since wrap is a QOL more than a "forced" solution we can merge this PR and I'll toy with it later to see what'd be the scope of the changes (if they prove to be worth it)

@georgesittas georgesittas merged commit 57722db into main Nov 11, 2024
6 checks passed
@georgesittas georgesittas deleted the jo/optionally_wrap_in_combine branch November 11, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants