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

Add LEFT, RIGHT and SUBSTRING SQL functions #26549

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Jun 16, 2023

Overview

Add LEFT, RIGHT and SUBSTRING SQL functions to APIv4

Before

no sense of direction

After

improved

Technical Details

LEFT() and RIGHT() are not strictly needed since the same results can be achieved with SUBSTRING(), but they are more intuitive and user-friendly. And SQL provides them all.

@colemanw : LEFT & RIGHT don't work well with '0' characters, but that is the default shown. Can we set a default value for the parameter?

Comments

@civibot
Copy link

civibot bot commented Jun 16, 2023

(Standard links)

@civibot civibot bot added the master label Jun 16, 2023
@demeritcowboy
Copy link
Contributor

Would SUBSTRING() be more general? You can do negative numbers with it to get the same as RIGHT.

@aydun
Copy link
Contributor Author

aydun commented Jun 16, 2023

Would SUBSTRING() be more general? You can do negative numbers with it to get the same as RIGHT.

@demeritcowboy SUBSTRING() is more general but less intuitive and user-friendly. So let's do both...

@aydun aydun changed the title Add LEFT & RIGHT SQL functions Add LEFT, RIGHT and SUBSTRING SQL functions Jun 16, 2023
@demeritcowboy
Copy link
Contributor

Ok sounds good.
And I agree that the default of 0 that appears would be nice to change.
And maybe out of scope since it's a general issue but for "Extract text" there's no labels on the parameters so it's not clear what it wants.

Also out of scope but unless I'm missing how to group-by on a transformed field this won't help the original stackexchange question. But the feature is still useful for other reasons.

@colemanw
Copy link
Member

Thanks for this @aydun. It would be helpful to add these to the test suite, can just add a few lines to the existing SqlFunctionTest.

This doesn't solve your UX default issue but I noticed a bug when handling 0 as an argument & pushed a fix: #26553

@aydun
Copy link
Contributor Author

aydun commented Jun 19, 2023

@colemanw - thanks, test added.

@colemanw colemanw merged commit 6a8ad50 into civicrm:master Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants