feat(spans): De-duplicate SQL conditions #2929
Conversation
| OR (a.id = %s OR a.org = %s) | ||
| OR (a.id = %s OR a.org = %s) | ||
| )"#, | ||
| "SELECT * FROM a WHERE status = %s OR ((id = %s OR org = %s))" |
There was a problem hiding this comment.
@gggritso this is not exactly the placeholder you suggested, but it does keep the implementation simple. Let me know if it's good enough.
There was a problem hiding this comment.
Works for me 👍🏻 going to defer to @Zylphrex on this, since he found the original issue
There was a problem hiding this comment.
This looks fine to me as it covers the case I originally raised.
jjbayer
changed the title
| OR (a.id = %s AND a.org = %s) | ||
| OR (a.id = %s AND a.org = %s) | ||
| )"#, | ||
| "SELECT * FROM a WHERE status = %s AND ((id = %s AND org = %s))" |
There was a problem hiding this comment.
are the double parentheses (( left on purpose ?
There was a problem hiding this comment.
I did not intend to leave them initially, but they might help indicate to the user that something was scrubbed here. They should be easy to remove though, if we want to @gggritso any opinion?
There was a problem hiding this comment.
I'd personally remove them. I think it's easy to notice queries are getting scrubbed so it's no surprise.
|
@Zylphrex what do you think? |
| OR (a.id = %s OR a.org = %s) | ||
| OR (a.id = %s OR a.org = %s) | ||
| )"#, | ||
| "SELECT * FROM a WHERE status = %s OR ((id = %s OR org = %s))" |
There was a problem hiding this comment.
This looks fine to me as it covers the case I originally raised.
| "SELECT count() FROM table1 WHERE id IN (%s, %s) AND id IN (%s, %s, %s)", | ||
| "SELECT count() FROM table1 WHERE id IN (%s) AND id IN (%s)" |
There was a problem hiding this comment.
I think this raises an edge case how queries like this one where we have the same condition twice joined by a AND operator.
Likely an uncommon use case, but the first list may be a subset of the second so it may be valid to transform it into just id IN (%s).
There was a problem hiding this comment.
I think it's OK to pretend that they are the same for grouping purposes, with the same reasoning as for the OR clause: If the left operand (id) and the operator IN repeat multiple times, it is likely a dynamically generated query that we can simplify.
#2929 partially scrubbed repeated conditions in SQL queries, but it did not consider the case where the repeated operands of an `OR` or `AND` operation are not siblings (see tree in code comments).
SQL queries are sometimes generated with dynamic conditions (
.. OR .. OR .. OR ..). After parameter scrubbing, these operands look the same, for exampleid = %s OR id = %s OR id = %s. In this case, simplify the condition toid = %s.ref: internal issue