You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2023/12/07 22:47:45 UTC
[PR] [MINOR] Document type limitations of SAFE_* functions [calcite]
mihaibudiu opened a new pull request, #3571:
URL: https://github.com/apache/calcite/pull/3571
I expected that I could use `SAFE_ADD(INTEGER, INTEGER)` to check for overflow. It turns out that doesn't work, because `SAFE_ADD` will convert the arguments to `BIGINT` first. I think this deserves to be documented. At least this is the BIGQUERY behavior: https://cloud.google.com/bigquery/docs/reference/standard-sql/mathematical_functions#safe_add
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "chucheng92 (via GitHub)" <gi...@apache.org>.
chucheng92 commented on code in PR #3571:
URL: https://github.com/apache/calcite/pull/3571#discussion_r1427594175
##########
site/_docs/reference.md:
##########
@@ -2806,14 +2806,14 @@ BigQuery's type system uses confusingly different names for types and functions:
| h s | string1 NOT RLIKE string2 | Whether *string1* does not match regex pattern *string2* (similar to `NOT LIKE`, but uses Java regex)
| b o | RPAD(string, length[, pattern ]) | Returns a string or bytes value that consists of *string* appended to *length* with *pattern*
| b o | RTRIM(string) | Returns *string* with all blanks removed from the end
-| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow
+| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow. Only supports BIGINT, DOUBLE, or DECIMAL arguments
Review Comment:
I think there will be ambiguity. The newly added comment will make users think that this function can only use BIGINT, DOUBLE, or DECIMAL. The internal transformation of integer is only internal processing.
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3571:
URL: https://github.com/apache/calcite/pull/3571#issuecomment-1846237311
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3571)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3571&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3571&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3571&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3571&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3571&metric=duplicated_lines_density&view=list) No Duplication information
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3571:
URL: https://github.com/apache/calcite/pull/3571#discussion_r1421498232
##########
site/_docs/reference.md:
##########
@@ -2806,14 +2806,14 @@ BigQuery's type system uses confusingly different names for types and functions:
| h s | string1 NOT RLIKE string2 | Whether *string1* does not match regex pattern *string2* (similar to `NOT LIKE`, but uses Java regex)
| b o | RPAD(string, length[, pattern ]) | Returns a string or bytes value that consists of *string* appended to *length* with *pattern*
| b o | RTRIM(string) | Returns *string* with all blanks removed from the end
-| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow
+| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow. Only supports BIGINT, DOUBLE, or DECIMAL arguments
Review Comment:
The fact that it does not throw was already in the message.
I want to emphasize that it does not work as you may expect for integers.
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3571:
URL: https://github.com/apache/calcite/pull/3571#discussion_r1421302810
##########
site/_docs/reference.md:
##########
@@ -2806,14 +2806,14 @@ BigQuery's type system uses confusingly different names for types and functions:
| h s | string1 NOT RLIKE string2 | Whether *string1* does not match regex pattern *string2* (similar to `NOT LIKE`, but uses Java regex)
| b o | RPAD(string, length[, pattern ]) | Returns a string or bytes value that consists of *string* appended to *length* with *pattern*
| b o | RTRIM(string) | Returns *string* with all blanks removed from the end
-| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow
+| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow. Only supports BIGINT, DOUBLE, or DECIMAL arguments
Review Comment:
In fact, you want to emphasize that It will not throw an error when it exceeds integer.
But for me, the comment is not helpful.
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu closed pull request #3571: [MINOR] Document type limitations of SAFE_* functions
URL: https://github.com/apache/calcite/pull/3571
--
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] Clarify semantics of SAFE* functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3571:
URL: https://github.com/apache/calcite/pull/3571#issuecomment-1965636235
I think I have addressed the comments, so I will merge this PR, because no one stepped to provide additional reviews. It only touches the documentation, and I think it's a clear improvement.
--
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] Clarify semantics of SAFE* functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3571:
URL: https://github.com/apache/calcite/pull/3571#issuecomment-1936433054
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3571) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3571&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3571&resolved=false&inNewCodePeriod=true)
 No data about Coverage
 No data about Duplication
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3571)
--
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] Clarify semantics of SAFE* functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3571:
URL: https://github.com/apache/calcite/pull/3571#issuecomment-1881977237
I have changed the descriptions of the SAFE_* functions to say: `Arguments are implicitly cast to one of the types BIGINT, DOUBLE, or DECIMAL`. I hope this is more precise.
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3571:
URL: https://github.com/apache/calcite/pull/3571#discussion_r1428356182
##########
site/_docs/reference.md:
##########
@@ -2806,14 +2806,14 @@ BigQuery's type system uses confusingly different names for types and functions:
| h s | string1 NOT RLIKE string2 | Whether *string1* does not match regex pattern *string2* (similar to `NOT LIKE`, but uses Java regex)
| b o | RPAD(string, length[, pattern ]) | Returns a string or bytes value that consists of *string* appended to *length* with *pattern*
| b o | RTRIM(string) | Returns *string* with all blanks removed from the end
-| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow
+| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow. Only supports BIGINT, DOUBLE, or DECIMAL arguments
Review Comment:
But this is exactly how all SQL functions work. For example, trigonometric functions are only defined for DOUBLE values. When you call them the compiler inserts an implicit cast for the argument to convert it to double. This kind of "internal processing" happens almost always, it has nothing to do with these functions. It is really part of the definition of SQL. These functions indeed can only use the types you enumerate, if you use a value with another type as argument it will be cast to one of these three types. That is exactly what I am documenting.
What is important in this case is to understand that one cannot use SAFE_* to perform safe computations on integers. If you do SAFE_ADD(int, int) the result will be a BIGINT. If you try to assign this to an integer column and it overflows you will get an exception, which is completely unexpected since this is what the SAFE function was designed to avoid. You also need a SAFE_CAST if you want to avoid the error.
--
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] Clarify semantics of SAFE* functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3571:
URL: https://github.com/apache/calcite/pull/3571
--
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] [MINOR] Document type limitations of SAFE_* functions [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3571:
URL: https://github.com/apache/calcite/pull/3571#discussion_r1442031507
##########
site/_docs/reference.md:
##########
@@ -2806,14 +2806,14 @@ BigQuery's type system uses confusingly different names for types and functions:
| h s | string1 NOT RLIKE string2 | Whether *string1* does not match regex pattern *string2* (similar to `NOT LIKE`, but uses Java regex)
| b o | RPAD(string, length[, pattern ]) | Returns a string or bytes value that consists of *string* appended to *length* with *pattern*
| b o | RTRIM(string) | Returns *string* with all blanks removed from the end
-| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow
+| b | SAFE_ADD(numeric1, numeric2) | Returns *numeric1* + *numeric2*, or NULL on overflow. Only supports BIGINT, DOUBLE, or DECIMAL arguments
Review Comment:
In BigQuery, all integer "types" are aliases for INT64.
https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types#integer_types
I think the docs could be improved but like @chucheng92 said, your suggestion implies other integer types are not supported. This is not the case.
`SELECT SAFE_ADD(CAST(32767 as SMALLINT), CAST(2 AS SMALLINT))` in BigQuery does not overflow. Your comments imply that it would.
If we are implementing a BigQuery function, my main concern is that we maintain parity with BigQuery.
The easiest way of doing this is coercing all other integer types to `BIGINT`. Whether "easiest" = "best" might be another discussion.
--
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