zio-sql icon indicating copy to clipboard operation
zio-sql copied to clipboard

PostgreSQL-specific `ltrim` and `rtrim`

Open jczuchnowski opened this issue 4 years ago • 4 comments

As @marekklis pointed out in https://github.com/zio/zio-sql/pull/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 avatar Dec 09 '20 00:12 jczuchnowski

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

marekklis avatar Dec 15 '20 09:12 marekklis

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 avatar Feb 23 '22 21:02 sviezypan

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)

SuperIzya avatar Apr 13 '22 08:04 SuperIzya

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?

hbibel avatar Apr 08 '23 08:04 hbibel