Skip to content

Comments

fix: no longer support the substring function#10242

Merged
alamb merged 6 commits intoapache:mainfrom
jonahgao:fix_substring
Apr 26, 2024
Merged

fix: no longer support the substring function#10242
alamb merged 6 commits intoapache:mainfrom
jonahgao:fix_substring

Conversation

@jonahgao
Copy link
Member

@jonahgao jonahgao commented Apr 26, 2024

Which issue does this PR close?

Closes #10240.

Rationale for this change

Before version 0.45.0 of sqlparser, the special flag for both substring('xxxx', 1, 2) and substring('xxxx' from 1 for 2) was false.

After apache/datafusion-sqlparser-rs#1173, the special flag for substring('xxxx', 1, 2) becomes true.

We need to support both formats as before, so there's no need to check the special flag.

The 'from' and 'for' parameters don't support string types because they should be treated as regular expressions. Currently, substring is just an alias ofsubstr. This PR adds tests to verify this.

Reference document: https://www.postgresql.org/docs/current/functions-string.html

What changes are included in this PR?

Re-enable substring('xxxx', 1, 2).

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Apr 26, 2024
@jonahgao jonahgao marked this pull request as draft April 26, 2024 03:15
@jonahgao jonahgao marked this pull request as ready for review April 26, 2024 03:29
Comment on lines +1875 to +1883
query T
SELECT substring('alphabet', 1)
----
alphabet

query T
SELECT substring('alphabet', 3, 2)
----
ph
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Looks good, nice catch 👍

@alamb
Copy link
Contributor

alamb commented Apr 26, 2024

Looks good to me @jonahgao -- thank you for the review @Jefffrey

@alamb alamb merged commit f8c623f into apache:main Apr 26, 2024
appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Apr 26, 2024
* fix: no longer support the `substring` function

* enable from-for format

* update test comment

* review feedback

* review feedback

Co-authored-by: Jeffrey Vo <[email protected]>

---------

Co-authored-by: Jeffrey Vo <[email protected]>
@jonahgao jonahgao deleted the fix_substring branch April 26, 2024 22:13
appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Apr 30, 2024
* fix: no longer support the `substring` function

* enable from-for format

* update test comment

* review feedback

* review feedback

Co-authored-by: Jeffrey Vo <[email protected]>

---------

Co-authored-by: Jeffrey Vo <[email protected]>
appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Apr 30, 2024
* fix: no longer support the `substring` function

* enable from-for format

* update test comment

* review feedback

* review feedback

Co-authored-by: Jeffrey Vo <[email protected]>

---------

Co-authored-by: Jeffrey Vo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression: no longer support the substring function

3 participants