Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: substr_index not handling negative occurrence correctly #9475

Merged
merged 4 commits into from
Mar 9, 2024

Conversation

jonahgao
Copy link
Member

@jonahgao jonahgao commented Mar 6, 2024

Which issue does this PR close?

Closes #9472.

Rationale for this change

Example query: select substr_index('arrow.apache.org', '.', -1).
Expected result is org, but the current result of main branch is e.org.

DataFusion CLI v36.0.0
❯ select substr_index('arrow.apache.org', '.', -1);
+------------------------------------------------------------+
| substr_index(Utf8("arrow.apache.org"),Utf8("."),Int64(-1)) |
+------------------------------------------------------------+
| e.org                                                      |
+------------------------------------------------------------+
1 row in set. Query took 0.006 seconds.

The previous test input was "www.apache.org", where "www" and "org" have the same length, which coincidentally led to the tests on negative occurrences being passed.

Background: Since this function originates from MySQL, its behavior should remain consistent with MySQL.

What changes are included in this PR?

Refactor the implementation of substr_index and fix bug.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Mar 6, 2024
});
}
}
// In MySQL, these cases will return an empty string.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like new implementation which looks more concise to me and easier to read.
About the MySQL comments, does that mean some cases are not the same between MySQL and Postgres?

Copy link
Member Author

Choose a reason for hiding this comment

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

About the MySQL comments, does that mean some cases are not the same between MySQL and Postgres?

No, actually there is no such function in PostgreSQL. The closest one is split_part, but their functionalities are different.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried it out here too https://www.w3schools.com/mysql/trymysql.asp?filename=trysql_func_mysql_substring_index2

And this behavior is consistent with what I observed

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.

Thanks @jonahgao -- I agree with @comphead the code in this PR is much nicer than what is on main and the tests are very nice

Thank you for the contribution


# Test substring_index using 'ac' as delimiter
query TIT
SELECT str, n, substring_index(str, 'ac', n) AS c FROM
Copy link
Contributor

Choose a reason for hiding this comment

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

the CTE style of these tests is very nice 👌

});
}
}
// In MySQL, these cases will return an empty string.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried it out here too https://www.w3schools.com/mysql/trymysql.asp?filename=trysql_func_mysql_substring_index2

And this behavior is consistent with what I observed

@alamb alamb merged commit 5537572 into apache:main Mar 9, 2024
23 checks passed
@jonahgao jonahgao deleted the substr_index branch March 9, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

substr_index doesn't behave correctly with a negative delimiter position
3 participants