You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "wasabii (via GitHub)" <gi...@apache.org> on 2026/09/12 15:35:07 UTC
[PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
wasabii opened a new pull request, #5259:
URL: https://github.com/apache/calcite/pull/5259
## Jira Link
[CALCITE-7778](https://issues.apache.org/jira/browse/CALCITE-7778)
## Changes Proposed
Parenthesize the `%` written for `MOD` where its lower precedence requires it. Shared in `RelToSqlConverterUtil` rather than the dialect, since `HiveSqlDialect` makes the same substitution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5259:
URL: https://github.com/apache/calcite/pull/5259#issuecomment-5646984484
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5259) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5259&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5259&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5259&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5259&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5259&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5259)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "wasabii (via GitHub)" <gi...@apache.org>.
wasabii commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009942767
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review Comment:
That's frankly probably a good thing to add! If done, we'd pass getLeftPrec and getRightPrec through that I guess.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5259:
URL: https://github.com/apache/calcite/pull/5259#issuecomment-6183896243
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009834109
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review Comment:
I wonder what happens if not all SQL dialects have the same operation precedence...
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review Comment:
I would shorten this comment to say that this will use parentheses when necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5259:
URL: https://github.com/apache/calcite/pull/5259#issuecomment-5670305011
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5259) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5259&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5259&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5259&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5259&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5259&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5259)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "wasabii (via GitHub)" <gi...@apache.org>.
wasabii commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009953831
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009031705
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
+ * 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,
Review Comment:
Don't you find that this would describe better what the function does?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r3996854879
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
+ * 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,
Review Comment:
maybe this should be called "unparseWithBinaryOperator"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "wasabii (via GitHub)" <gi...@apache.org>.
wasabii commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009017183
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
+ * 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,
Review Comment:
Is that a change you want me to make?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009991330
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "wasabii (via GitHub)" <gi...@apache.org>.
wasabii commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009924742
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review Comment:
Should be fine. That was the goal. The parenethesis are added if required to meet the requirements of the original precedents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "wasabii (via GitHub)" <gi...@apache.org>.
wasabii commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009924742
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review Comment:
Should be fine. That was the goal. The parenethesis are added if required to meet the requirements of the original precedents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7778] JDBC adapter for MSSQL generates % for MOD without preserving grouping, giving wrong results [calcite]
Posted by "wasabii (via GitHub)" <gi...@apache.org>.
wasabii commented on code in PR #5259:
URL: https://github.com/apache/calcite/pull/5259#discussion_r4009938382
##########
core/src/main/java/org/apache/calcite/util/RelToSqlConverterUtil.java:
##########
@@ -339,6 +341,26 @@ public static void unparseBoolLiteralToCondition(SqlWriter writer, boolean value
writer.endList(frame);
}
+ /**
+ * 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
Review Comment:
Upon staring at it longer, I don't think dialects have any ability to hold per operator precedence, do they?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org