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

fix(spans): Apply max. SQL expression depth after simplification #3003

Merged
merged 5 commits into from
Jan 30, 2024

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Jan 25, 2024

Repeating OR/AND conditions are collapsed, but if the maximum query depth is reached, the limit is applied before we can collapse it. This leads to scrubbed queries like:

WHERE ((.. OR (..) OR (.. = %s AND b = %s) OR (a = %s AND b = %s))

Fix by splitting the query visitor into two steps (first normalize, then reduce depth).

This PR also removes redundant parentheses around binary operations (e.g. a OR (b OR c)).

ref: internal issue

@jjbayer jjbayer changed the title test(spans): max depth applied before simplification fix(spans): Apply max. SQL expression depth after simplification Jan 25, 2024
@jjbayer jjbayer marked this pull request as ready for review January 26, 2024 12:40
@jjbayer jjbayer requested a review from a team as a code owner January 26, 2024 12:40
@jjbayer jjbayer requested review from gggritso and Zylphrex January 26, 2024 12:40
Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

👍🏻 makes sense, though I guess we're losing the ( and ) that used to surround collapsed conditions, is that right?

…arser.rs

Co-authored-by: Oleksandr <1931331+olksdr@users.noreply.github.com>
@jjbayer
Copy link
Member Author

jjbayer commented Jan 29, 2024

👍🏻 makes sense, though I guess we're losing the ( and ) that used to surround collapsed conditions, is that right?

@gggritso Yes, we're losing that. Because this normalization is quite general, there might also be a bunch of other cases where we now remove parentheses, i.e. there will be grouping changes. If you feel strongly about I can revert this part, but it will result in duplicate groups for the case we're trying to fix.

@gggritso
Copy link
Member

@jjbayer 🚢 👍🏻

@jjbayer jjbayer merged commit bf9327b into master Jan 30, 2024
20 checks passed
@jjbayer jjbayer deleted the fix/spans-sql-long-or branch January 30, 2024 08:18
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.

4 participants