You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "saurabhd336 (via GitHub)" <gi...@apache.org> on 2023/09/04 04:50:59 UTC
[GitHub] [pinot] saurabhd336 opened a new pull request, #11502: Change substring to comply with standard sql definition
saurabhd336 opened a new pull request, #11502:
URL: https://github.com/apache/pinot/pull/11502
TPCH's query 24 https://github.com/apache/pinot/blob/master/pinot-integration-tests/src/test/resources/tpch/24.sql fails due to pinot's `substring` definition being different from standard sql definition. As per https://www.postgresql.org/docs/15/functions-string.html
```
substring ( string text [ FROM start integer ] [ FOR count integer ] ) → text
Extracts the substring of string starting at the start'th character if that is specified, and stopping after count characters if that is specified. Provide at least one of start and count.
substring('Thomas' from 2 for 3) → hom
substring('Thomas' from 3) → omas
substring('Thomas' for 2) → Th
```
This change fixes the tpch query, but introduces a backward-incopatible change. Will wait for reviews.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 merged pull request #11502: Change substring to comply with standard sql definition
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 merged PR #11502:
URL: https://github.com/apache/pinot/pull/11502
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1705469098
If this is new syntax, then it won't break the backward compatibility right?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1706068678
@xiangfu0 yes calcite sql parser itself parses `substring ( string text [ FROM start integer ] [ FOR count integer ] )` into the correct substring function node
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11502: Change substring to comply with standard sql definition
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #11502:
URL: https://github.com/apache/pinot/pull/11502#discussion_r1315100957
##########
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/StringFunctions.java:
##########
@@ -79,29 +79,27 @@ public static String upper(String input) {
/**
* @see String#substring(int)
* @param input Parent string
- * @param beginIndex index from which substring should be created
+ * @param beginIndex 1 based index from which substring should be created
* @return substring from beginIndex to end of the parent string
*/
@ScalarFunction(names = {"substr", "substring"})
Review Comment:
Shall we consider keep `substr` as current behavior and use `substring` for postgresql behavior?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1705960472
@xiangfu0 sure let's go with having both substr and substring as separate functions
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1704636224
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11502?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11502](https://app.codecov.io/gh/apache/pinot/pull/11502?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9464c6e) into [master](https://app.codecov.io/gh/apache/pinot/commit/a485778a8ebf684bb38b8c596c60bd6df4598c73?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (a485778) will **decrease** coverage by `48.60%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11502 +/- ##
=============================================
- Coverage 63.04% 14.44% -48.60%
+ Complexity 1108 201 -907
=============================================
Files 2320 2320
Lines 124520 124518 -2
Branches 19011 19011
=============================================
- Hits 78507 17990 -60517
- Misses 40422 105005 +64583
+ Partials 5591 1523 -4068
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.44% <0.00%> (-48.57%)` | :arrow_down: |
| [java-17](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-20](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.44% <0.00%> (-48.60%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.44% <0.00%> (-48.60%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11502/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `14.44% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pinot/pull/11502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../pinot/common/function/scalar/StringFunctions.java](https://app.codecov.io/gh/apache/pinot/pull/11502?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vc2NhbGFyL1N0cmluZ0Z1bmN0aW9ucy5qYXZh) | `0.00% <0.00%> (-67.55%)` | :arrow_down: |
... and [1497 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11502/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] saurabhd336 commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1706326191
@xiangfu0 , tpch's query 24 already tests that https://github.com/apache/pinot/blob/master/pinot-integration-tests/src/test/resources/tpch/24.sql hence i didn't add an explicit one.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1706041378
Do we support syntax like: `substring ( string text [ FROM start integer ] [ FOR count integer ] )` in calcite?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1706284428
> @xiangfu0 yes calcite sql parser itself parses `substring ( string text [ FROM start integer ] [ FOR count integer ] )` into the correct substring function node
Nice, then please add a test for that as well.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on pull request #11502: Change substring to comply with standard sql definition
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #11502:
URL: https://github.com/apache/pinot/pull/11502#issuecomment-1704764522
I remember discussing this for Tableau as well. The reason it wasn't done is cause it breaks backward compatibility for people already using this function.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org