Skip to content

Conversation

@adsharma
Copy link
Contributor

@adsharma adsharma commented Aug 25, 2021

Which issue does this PR close?

Closes #935

Rationale for this change

What changes are included in this PR?

Implements the trim ( [ LEADING | TRAILING | BOTH ] [ FROM ] string text [, characters text ] ) syntax

Are there any user-facing changes?

Yes

@github-actions github-actions bot added datafusion sql SQL Planner labels Aug 25, 2021
@seddonm1
Copy link
Contributor

hi @adsharma . this looks good and I think it is correct.

I am not sure your tests are proving anything additional? This would benefit from tests proving the trim ( [ LEADING | TRAILING | BOTH ] [ FROM ] string text [, characters text ] ) syntax. Maybe you could remove these tests and add some to tests/sql.rs which have ensure the SQL string -> function coverage like trim(both from 'yxTomxx', 'xyz').

@seddonm1
Copy link
Contributor

looks good 👍

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thank you for the contribution @adsharma and the review @seddonm1 👍

For anyone who is curious, it turns out that mysql and postgres differ in their treatment of TRIM but this PR follows the Postgres semantics which is the design goal for DataFusion. More details on #935 (comment)

@alamb alamb changed the title Add support for trim variants Add support for TRIM LEADING/TRAILING/BOTH syntax Aug 26, 2021
@alamb alamb added the enhancement New feature or request label Aug 26, 2021
@alamb
Copy link
Contributor

alamb commented Aug 29, 2021

Thanks again @adsharma -- this is a great first time contribution!

@alamb alamb merged commit 4d568d2 into apache:master Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for TRIM BOTH/LEADING/TRAILING

4 participants