You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "normanj-bitquill (via GitHub)" <gi...@apache.org> on 2024/07/18 20:05:30 UTC
[PR] [CALCITE-6483] Add length function to PostgreSQL function library [calcite]
normanj-bitquill opened a new pull request, #3865:
URL: https://github.com/apache/calcite/pull/3865
Enabled the LEN function for:
* Amazon Redshift
* Snowflake
* Spark
Enabled the LENGTH function for:
* BigQuery
* Hive
* PostgreSQL
* Amazon Redshift
* Snowflake
* Spark
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3865:
URL: https://github.com/apache/calcite/pull/3865#discussion_r1684086095
##########
site/_docs/reference.md:
##########
@@ -2803,8 +2803,8 @@ In the following:
| m | JSON_STORAGE_SIZE(jsonValue) | Returns the number of bytes used to store the binary representation of *jsonValue*
| b o p r s | LEAST(expr [, expr ]* ) | Returns the least of the expressions
| b m p r s | LEFT(string, length) | Returns the leftmost *length* characters from the *string*
-| f s | LEN(string) | Equivalent to `CHAR_LENGTH(string)`
-| b f s | LENGTH(string) | Equivalent to `CHAR_LENGTH(string)`
+| f r s | LEN(string) | Equivalent to `CHAR_LENGTH(string)`
+| b f h p r s | LENGTH(string) | Equivalent to `CHAR_LENGTH(string)`
Review Comment:
I think the `r` (from redshift) slipped in this line by mistake?
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2238796456
> @normanj-bitquill overall LGTM. I think that current patch is wider than the original intent, so perhaps we should adjust the Jira title (and commit message) to something broader, e.g. "Add LEN and LENGTH functions to missing libraries", wdyt?
@rubenada Thank you for your reply. I am worried that in some negative tests, the results of each dialect may be different. Do you mind if I test it on some databases first?
--
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-6483] Enable LEN and LENGTH in the correct function libraries [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada merged PR #3865:
URL: https://github.com/apache/calcite/pull/3865
--
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-6483] Enable LEN and LENGTH in the correct function libraries [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2241051525
Thanks @normanj-bitquill , LGTM. I'll merge in the next 24-48h if no further comment appears.
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on code in PR #3865:
URL: https://github.com/apache/calcite/pull/3865#discussion_r1684966079
##########
site/_docs/reference.md:
##########
@@ -2803,8 +2803,8 @@ In the following:
| m | JSON_STORAGE_SIZE(jsonValue) | Returns the number of bytes used to store the binary representation of *jsonValue*
| b o p r s | LEAST(expr [, expr ]* ) | Returns the least of the expressions
| b m p r s | LEFT(string, length) | Returns the leftmost *length* characters from the *string*
-| f s | LEN(string) | Equivalent to `CHAR_LENGTH(string)`
-| b f s | LENGTH(string) | Equivalent to `CHAR_LENGTH(string)`
+| f r s | LEN(string) | Equivalent to `CHAR_LENGTH(string)`
+| b f h p r s | LENGTH(string) | Equivalent to `CHAR_LENGTH(string)`
Review Comment:
Amazon Redshift supports both `LEN()` and `LENGTH()`.
https://docs.aws.amazon.com/redshift/latest/dg/r_LEN.html
The Redshift function library extends the PostgreSQL function library and by default has all PostgreSQL functions (unless specifically excluded).
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2238721010
> Should we add tests to SqlOperatorTest?
@caicancai I think there is no need to touch `SqlOperatorTest`. The way that class works, the already existing tests `testLenFunc` and `testLengthFunc` will automatically consider the libraries in the `@LibraryOperator` annotation of `LEN` and `LENGTH` (including the newly added ones). So new checks are added transparently for the new libraries.
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2238724961
@normanj-bitquill overall LGTM. I think that current patch is wider than the original intent, so perhaps we should adjust the Jira title (and commit message) to something broader, e.g. "Add LEN and LENGTH functions to missing libraries", wdyt?
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2237511419
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3865) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3865&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3865&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3865&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3865&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3865&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3865)
--
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-6483] Enable LEN and LENGTH in the correct function libraries [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2240179136
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3865) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3865&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3865&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3865&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3865&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3865&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3865)
--
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-6483] Enable LEN and LENGTH in the correct function libraries [calcite]
Posted by "normanj-bitquill (via GitHub)" <gi...@apache.org>.
normanj-bitquill commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2240107466
@rubenada I have updated the ticket title and commit message. Let me know if it still doesn't match the change.
--
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-6483] Enable LEN and LENGTH in the correct function libraries [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3865:
URL: https://github.com/apache/calcite/pull/3865#discussion_r1685328041
##########
site/_docs/reference.md:
##########
@@ -2803,8 +2803,8 @@ In the following:
| m | JSON_STORAGE_SIZE(jsonValue) | Returns the number of bytes used to store the binary representation of *jsonValue*
| b o p r s | LEAST(expr [, expr ]* ) | Returns the least of the expressions
| b m p r s | LEFT(string, length) | Returns the leftmost *length* characters from the *string*
-| f s | LEN(string) | Equivalent to `CHAR_LENGTH(string)`
-| b f s | LENGTH(string) | Equivalent to `CHAR_LENGTH(string)`
+| f r s | LEN(string) | Equivalent to `CHAR_LENGTH(string)`
+| b f h p r s | LENGTH(string) | Equivalent to `CHAR_LENGTH(string)`
Review Comment:
True! I hadn't realized. Thanks for the clarification.
--
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-6483] Add length function to PostgreSQL function library [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #3865:
URL: https://github.com/apache/calcite/pull/3865#issuecomment-2238256398
Should we add tests to SqlOperatorTest?
--
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