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

PostgreSQL-specific ltrim and rtrim #341

Open
jczuchnowski opened this issue Dec 9, 2020 · 4 comments
Open

PostgreSQL-specific ltrim and rtrim #341

jczuchnowski opened this issue Dec 9, 2020 · 4 comments
Labels

Comments

@jczuchnowski
Copy link
Member

As @marekklis pointed out in #339, it appears that PostgreSQL can take an additional, optional parameter in ltrim and rtrim functions.
Just adding these functions in PostgresModule might cause import clashes. We need to figure out the way to deal with such situation.

@jczuchnowski jczuchnowski added help wanted Extra attention is needed PostgreSQL labels Dec 9, 2020
@marekklis
Copy link
Contributor

marekklis commented Dec 15, 2020

Probably we should move these functions from the core module to modules specific for every database servers.

@sviezypan
Copy link
Collaborator

This is still an open issue. Research is needed to find out if ltrim and rtrim behave differently on different databases. If yes, then move them to specific modules and implement accordingly.

@sviezypan sviezypan added good first issue Good for newcomers and removed help wanted Extra attention is needed labels Feb 23, 2022
@SuperIzya
Copy link

It would be best if there is a way to override in the DB-specific module the default implementation from core. That way, even if ltrim is behaving the same for MySQL, MSSQL, Oracle and Postgre, but differently for XXX - it will be easy to add XXX db support without rewriting the core. (Just stating the obvious)

hbibel added a commit to hbibel/zio-sql that referenced this issue Apr 8, 2023
@hbibel
Copy link
Contributor

hbibel commented Apr 8, 2023

Probably we should move these functions from the core module to modules specific for every database servers.

The unary variants of ltrim and rtrim behave the same for all supported DMBS. There is however a difference in the two-argument variant:

  • MySQL: Returns the string str with leading space characters removed. Returns NULL if str is NULL.
  • Oracle: LTRIM removes from the left end of char all of the characters contained in set. If you do not specify set, then it defaults to a single blank.
  • PostgreSQL: Removes the longest string containing only characters in characters (a space by default) from the start of string.
  • SQL Server: Returns a character expression with a type of string argument where the space character char(32) or other specified characters are removed from the beginning of a character_expression.

So I'd leave Ltrim and Rtrim in the core module, but add Ltrim2 and Rtrim2 in all submodules that support it.

Note that while SQL Server since 2022 (compatibility level 160) supports ltrim and rtrim with two args, the Azure SQL Edge image used in tests doesn't because it has compatibility level 150. I presume this one is used because the SQL Server image doesn't run on M1 without workarounds. Therefore I omitted it for now. If the two-arg variant is needed for SQL Server then I guess one needs to implement some tests based on the mcr.microsoft.com/mssql/server:2022-latest image and disable them on M1 machines.

Side note to the naming: There is some inconsistency in the casing of Ltrim and LPad. I personally prefer LTrim because it's written how you pronounce it ("l-trim"). But for consistency I named the new functions Ltrim2 and Rtrim2. Would it be okay if I rename it (a breaking change)? Should I add an alias and deprecate the other one? Or should we leave it as it is? WDYT?

jczuchnowski added a commit that referenced this issue Jun 9, 2023
#341 - Implement Ltrim2 and Rtrim2 for Postgres and Oracle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants