Conversation
…serving grouping, giving wrong results
| * operator before the dialect is consulted, so an operator that binds less | ||
| * tightly needs them added here. | ||
| */ | ||
| public static void unparseWithOperator(SqlWriter writer, SqlOperator operator, |
There was a problem hiding this comment.
maybe this should be called "unparseWithBinaryOperator"
There was a problem hiding this comment.
Is that a change you want me to make?
There was a problem hiding this comment.
Don't you find that this would describe better what the function does?
|
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
You can squash the commits.
I left one more suggestion for improving the comments.
| * Writes a two-operand call with an operator other than its own, | ||
| * parenthesized as that operator requires. | ||
| * | ||
| * <p>{@link SqlCall#unparse} chooses the parentheses from the call's own |
There was a problem hiding this comment.
I would shorten this comment to say that this will use parentheses when necessary.
There was a problem hiding this comment.
I wonder what happens if not all SQL dialects have the same operation precedence...
There was a problem hiding this comment.
Upon staring at it longer, I don't think dialects have any ability to hold per operator precedence, do they?
There was a problem hiding this comment.
That's frankly probably a good thing to add! If done, we'd pass getLeftPrec and getRightPrec through that I guess.
There was a problem hiding this comment.
Actually. There are a bunch of special operators floating around for specific SQL languages. I guess if this got hit after those were substituted in, it would be fine, if those carried their own special precedence.
There was a problem hiding this comment.
In calcite operators cannot change precedences, but I am saying is that the calcite unparse will decide whether to use parens based on the Calcite rules, which may not match the target dialect rules.
2 participants
Jira Link
CALCITE-7778
Changes Proposed
Parenthesize the
%written forMODwhere its lower precedence requires it. Shared inRelToSqlConverterUtilrather than the dialect, sinceHiveSqlDialectmakes the same substitution and somebody might want to fix that also some day.