You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/01/05 22:44:04 UTC

[GitHub] [arrow] ovr opened a new pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

ovr opened a new pull request #9108:
URL: https://github.com/apache/arrow/pull/9108


   Hello!
   
   Thanks


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755777874


   I agree with @seddonm1  that ideally we would follow Postgres when possible so we can just re-use all their documentation and semantics and so we don't end up inadvertently creating yet another SQL dialect like spark. 
   
   Specifically in this case, it looks like the relevant postgres function is called `TRIM`. A screen shot from https://www.postgresql.org/docs/9.1/functions-string.html
   
   ![Screen Shot 2021-01-06 at 6 29 19 PM](https://user-images.githubusercontent.com/490673/103832756-19c55a00-504d-11eb-8d89-840c16f2b788.png)
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755036138


   These were not added previously as technically they are not in the ANSI SQL standards. Hopefully this can drive a decision on what dialect to support.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] andygrove commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755791235


   I am also +1 for choosing Postgres dialect as the default. 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] jorgecarleitao commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755451875


   @andygrove @alamb , opinions?
   
   My (weak) opinion is that as long as we do not conflict with ANSI SQL (in that we offer a conflicting or incomplete spec), I do not see any problem.
   
   
   
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io edited a comment on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755256302


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=h1) Report
   > Merging [#9108](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=desc) (30465a8) into [master](https://codecov.io/gh/apache/arrow/commit/fdf5e88a67f33c0a76673a32938274f063c9cb93?el=desc) (fdf5e88) will **decrease** coverage by `0.00%`.
   > The diff coverage is `63.63%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9108/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9108      +/-   ##
   ==========================================
   - Coverage   82.57%   82.57%   -0.01%     
   ==========================================
     Files         204      204              
     Lines       50327    50349      +22     
   ==========================================
   + Hits        41560    41574      +14     
   - Misses       8767     8775       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `76.90% <ø> (ø)` | |
   | [...datafusion/src/physical\_plan/string\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3N0cmluZ19leHByZXNzaW9ucy5ycw==) | `87.50% <ø> (ø)` | |
   | [rust/datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3Rlc3RzL3NxbC5ycw==) | `99.82% <ø> (ø)` | |
   | [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `78.51% <63.63%> (-1.49%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=footer). Last update [fdf5e88...30465a8](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] github-actions[bot] commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-754947112


   https://issues.apache.org/jira/browse/ARROW-11138


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-756195398


   Filed https://issues.apache.org/jira/browse/ARROW-11164 for the optional second argument
   
   Filed https://issues.apache.org/jira/browse/ARROW-11165 to document the desired dialect -- I'll try and create a proposal and send a note to the mailing list


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] alamb closed pull request #9108: ARROW-11138: [Rust] [DataFusion] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9108:
URL: https://github.com/apache/arrow/pull/9108


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755792827


   > I agree with @seddonm1 that ideally we would follow Postgres when possible so we can just re-use all their documentation and semantics and so we don't end up inadvertently creating yet another SQL dialect like spark.
   > 
   > Specifically in this case, it looks like the relevant postgres function is called `TRIM`. A screen shot from https://www.postgresql.org/docs/9.1/functions-string.html
   
   FYI (because I had to work this out) the way to read the Postgres documentation the first table (Table 9-6. SQL String Functions and Operators) is the ANSI SQL and the second table (Table 9-7. Other String Functions) are Postgres dialect specific implementations. This pattern is standardised for other Postgres documentation.
   
   You can see that `ltrim` comes from the Table 9-7. Other String Functions section.
   
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] seddonm1 commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755682309


   Thanks @jorgecarleitao and @ovr 
   
   My view would be to elect an initial dialect (my preference being Postgres - https://www.postgresql.org/docs/13/functions-string.html which supports ANSI + what appear to be sensible additions), document that in the readme and assess any function PRs against that. The secondary benefit is that we can point to the Postgres (or other) docs for usage instructions. 
   
   The risk of not making the decision early risks what happened with Spark SQL where [functions](https://spark.apache.org/docs/latest/api/sql/index.html) were added seemingly ad-hoc making their usage very difficult and no clear feature matrix available.
   
   I am happy to volunteer to update documentation if we can reach the consensus on dialect.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] ovr commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755449354


   @seddonm1 
   
   > Hopefully this can drive a decision on what dialect to support.
   
   Should I create an issue to discuss it or mailing list? Is there any channel for communications like slack?
   
   @jorgecarleitao 
   
   Thanks. Done ✅ 


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] codecov-io commented on pull request #9108: ARROW-11138: [Rust] Add ltrim, rtrim to built-in functions

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9108:
URL: https://github.com/apache/arrow/pull/9108#issuecomment-755256302


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=h1) Report
   > Merging [#9108](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=desc) (22e09cc) into [master](https://codecov.io/gh/apache/arrow/commit/fdf5e88a67f33c0a76673a32938274f063c9cb93?el=desc) (fdf5e88) will **decrease** coverage by `0.00%`.
   > The diff coverage is `63.63%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9108/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9108      +/-   ##
   ==========================================
   - Coverage   82.57%   82.57%   -0.01%     
   ==========================================
     Files         204      204              
     Lines       50327    50349      +22     
   ==========================================
   + Hits        41560    41574      +14     
   - Misses       8767     8775       +8     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `76.90% <ø> (ø)` | |
   | [...datafusion/src/physical\_plan/string\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3N0cmluZ19leHByZXNzaW9ucy5ycw==) | `87.50% <ø> (ø)` | |
   | [rust/datafusion/tests/sql.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3Rlc3RzL3NxbC5ycw==) | `99.82% <ø> (ø)` | |
   | [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9108/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `78.51% <63.63%> (-1.49%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=footer). Last update [fdf5e88...30465a8](https://codecov.io/gh/apache/arrow/pull/9108?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org