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

FIND_IN_SET to PATINDEX translation missing %s #339

Open
rmc47 opened this Issue Mar 28, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@rmc47
Copy link
Contributor

rmc47 commented Mar 28, 2019

At the moment, the FIND_IN_SET translation (used by bbPress's topic subscription notifications) turns into PATINDEX:
https://github.com/ProjectNami/projectnami/blob/master/wp-includes/translations.php#L1151

However, the pattern argument isn't surrounded by %s, which https://docs.microsoft.com/en-us/sql/t-sql/functions/patindex-transact-sql suggests it must be, so returns no results:

SELECT PATINDEX(',456,',',123,456,789,')
-- returns 0

SELECT PATINDEX('%,456,%',',123,456,789,')
-- returns 5

I'll submit a PR to fix this, but @LitKnd pointed out that since SQL 2016, there's the rather nice STRING_SPLIT function which might offer a neater solution. What's your feeling on how many folks would be hit by upping the current SQL 2012 requirement?

@patrickebates

This comment has been minimized.

Copy link
Member

patrickebates commented Apr 5, 2019

To answer your question regarding SQL 2016 commands
https://projectnami.org/sql-edition-usage-stats-for-2018/

Looks like 23.5% of installations run on a version below 2016.

Perhaps a version check to switch between the two solutions?

We're not opposed to using newer commands in certain use cases. For example, there is no way to translate a GROUP_CONCAT until SQL 2017 and STRING_AGG. Sooner or later we will need to implement it for a number of plugins out there.

@rmc47

This comment has been minimized.

Copy link
Contributor Author

rmc47 commented Apr 5, 2019

Thanks for digging into it. I wouldn't worry about STRING_SPLIT for now - Kendra did some benchmarking, and performance seemed indistinguishable from PATINDEX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.