You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/07 06:12:43 UTC
[GitHub] [druid] gianm opened a new pull request, #13037: SQL: Fix round-trips of floating point literals.
gianm opened a new pull request, #13037:
URL: https://github.com/apache/druid/pull/13037
When writing RexLiterals into Druid expressions, we now write non-integer numeric literals in such a way that ensures they are parsed as doubles on the other end.
Fixes behavior where, in cases where literals are embedded in expressions, casts like `CAST(3 AS DOUBLE)` seemingly get "lost". I noticed this when doing `STRING_FORMAT('%.2f', CAST(3 AS DOUBLE))`. The expression was generated as `format('%.2f', 3)`, which led to an error since `f` expects floating point input. I bet other expressions could have had odd behavior too.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13037: Expressions: fixes for round-trips of floating point literals, Long.MIN_VALUE literals, Shuffle.visitAll.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #13037:
URL: https://github.com/apache/druid/pull/13037#discussion_r965435612
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java:
##########
@@ -104,9 +104,24 @@ private static String escape(final String s)
return escaped.toString();
}
- public static String numberLiteral(final Number n)
+ public static String longLiteral(final long n)
{
- return n == null ? nullLiteral() : n.toString();
+ return String.valueOf(n);
+ }
+
+ public static String doubleLiteral(final double n)
+ {
+ final String s = String.valueOf(n);
+
+ // Ensure number is parsed as a double: add ".0" if necessary.
+ for (int i = 0; i < s.length(); i++) {
+ final char c = s.charAt(i);
+ if (!Character.isDigit(c)) {
Review Comment:
The latest patch fixes two additional bugs that I noticed in the rabbit hole that this led me down. I updated the PR description to describe all three bugs.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13037: SQL: Fix round-trips of floating point literals.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #13037:
URL: https://github.com/apache/druid/pull/13037#discussion_r965402243
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java:
##########
@@ -104,9 +104,24 @@ private static String escape(final String s)
return escaped.toString();
}
- public static String numberLiteral(final Number n)
+ public static String longLiteral(final long n)
{
- return n == null ? nullLiteral() : n.toString();
+ return String.valueOf(n);
+ }
+
+ public static String doubleLiteral(final double n)
+ {
+ final String s = String.valueOf(n);
+
+ // Ensure number is parsed as a double: add ".0" if necessary.
+ for (int i = 0; i < s.length(); i++) {
+ final char c = s.charAt(i);
+ if (!Character.isDigit(c)) {
Review Comment:
This comment is absolutely correct and led me down a crazy rabbit hole 🐰
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #13037: Expressions: fixes for round-trips of floating point literals, Long.MIN_VALUE literals, Shuffle.visitAll.
Posted by GitBox <gi...@apache.org>.
gianm merged PR #13037:
URL: https://github.com/apache/druid/pull/13037
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13037: SQL: Fix round-trips of floating point literals.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13037:
URL: https://github.com/apache/druid/pull/13037#discussion_r965152285
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java:
##########
@@ -104,9 +104,24 @@ private static String escape(final String s)
return escaped.toString();
}
- public static String numberLiteral(final Number n)
+ public static String longLiteral(final long n)
{
- return n == null ? nullLiteral() : n.toString();
+ return String.valueOf(n);
+ }
+
+ public static String doubleLiteral(final double n)
+ {
+ final String s = String.valueOf(n);
+
+ // Ensure number is parsed as a double: add ".0" if necessary.
+ for (int i = 0; i < s.length(); i++) {
+ final char c = s.charAt(i);
+ if (!Character.isDigit(c)) {
Review Comment:
i think you need to check for leading `-` too?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #13037: SQL: Fix round-trips of floating point literals.
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #13037:
URL: https://github.com/apache/druid/pull/13037#discussion_r965404372
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java:
##########
@@ -104,9 +104,24 @@ private static String escape(final String s)
return escaped.toString();
}
- public static String numberLiteral(final Number n)
+ public static String longLiteral(final long n)
{
- return n == null ? nullLiteral() : n.toString();
+ return String.valueOf(n);
+ }
+
+ public static String doubleLiteral(final double n)
+ {
+ final String s = String.valueOf(n);
+
+ // Ensure number is parsed as a double: add ".0" if necessary.
+ for (int i = 0; i < s.length(); i++) {
+ final char c = s.charAt(i);
+ if (!Character.isDigit(c)) {
Review Comment:
I discovered a couple of things.
First: there is no need to do any of this checking, since simply using `String.valueOf(double)` is enough: it adds the extra `.0` for whole numbers anyway. The real fix to the bug was replacing `numberLiteral` with `longLiteral` and `doubleLiteral`, as this forces us to check the desired type, which means we don't lose the cast. (Previously: we'd lose casts by passing long `3` to `numberLiteral` even though the desired type was DOUBLE.) So, we can simplify the code here, which is nice.
Second: I tried to add a test for `Long.MIN_VALUE` and found that we cannot parse it! It sort of makes sense. To avoid ambiguity between unary minus and negative literals, we don't actually have negative literals. We represent "negative literals" as unary minus next to a positive literal of the absolute value. But! That means `Long.MIN_VALUE` can't be represented, since the absolute value can't fit into a `long` and therefore can't go into a LongExpr. I'm going to try to fix this and see where it leads me. This is where the 🐰 comes in…
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org