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.

Sign up for GitHub

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

Jump to bottom

string.PadLeft and string.PadRight wrong translation #3116

Open
ozgur-yalcin opened this issue Feb 26, 2024 · 5 comments · May be fixed by #3288
Open

string.PadLeft and string.PadRight wrong translation #3116

ozgur-yalcin opened this issue Feb 26, 2024 · 5 comments · May be fixed by #3288
Labels
bug Something isn't working
Milestone
10.0.0

Comments

@ozgur-yalcin
Copy link

ozgur-yalcin commented Feb 26, 2024 •
edited
Loading

Npgsql EF Core provider automatically translates stringValue.PadLeft(length, char) -> lpad(stringValue, length, char).

However, these two functions do not produce exactly the same result and may cause problems.

C# example:

        var stringValue = "1234";
        Console.WriteLine(stringValue.PadLeft(2, '0'));
        /* Output : 1234 */

SQL example:

        SELECT lpad('1234', 2, '0')
        /* Output : 12 */

If the string is already longer than length then it is truncated in lpad and rpad functions.
lpad and rpad functions need to be replaced with a new function that gives the correct result.

I tried a method like the one below, but strangely, in a different scenario, I still did not get the result I expected.
SQL example:

        SELECT greatest('1234'::text, lpad('1234'::text, 6, '0')::text)
        /* Output : 1234 */

After working on it a bit I found a method that works correctly as follows

         /* lpad alternative */
        SELECT repeat('0', 6 - length('1234')) || '1234' /* Output : 001234 */
        SELECT repeat('0', 2 - length('1234')) || '1234' /* Output : 1234 */
        
        /* rpad alternative */
        SELECT '1234' || repeat('0', 6 - length('1234')) /* Output : 123400 */
        SELECT '1234' || repeat('0', 2 - length('1234')) /* Output : 1234 */

Do you have any other ideas?
@ozgur-yalcin ozgur-yalcin changed the title lpad and rpad wrong translation string.PadLeft and string.PadRight wrong translation Feb 26, 2024
@Voonti
Copy link

Voonti commented Apr 11, 2024

image
image

It seems like, this is intentional. Better question is, what is the origin of padding, to truncated if less, or to return it intact.

@roji
Copy link
Member

roji commented Apr 12, 2024

@ozgur-yalcin sorry for not answering earlier, this is a very busy period.

If the behavior discrepancy is when the string is longer, then we can continue to use lpad/rpad, but wrap it in a CASE/WHEN in the translation to simply return the original string when it's longer, i.e. CASE length(x) >= y THEN x ELSE lpad(x, y, '0'), how does that sound?

@Voonti the point @ozgur-yalcin made above is that .NET's PadLeft and PG lpad don't behave the same way when the string a larger (i.e. PG truncates, .NET does nothing).

@roji
Copy link
Member

roji commented Apr 12, 2024

@ozgur-yalcin if this sounds good, are you interested in submitting a PR to fix this?

@roji roji added the bug Something isn't working label Apr 12, 2024
@roji roji added this to the 9.0.0 milestone Apr 12, 2024
@ozgur-yalcin
Copy link
Author

Sounds good, thanks for your support.

@ozgur-yalcin ozgur-yalcin reopened this Apr 18, 2024
@ChrisJollyAU
Copy link
Contributor

@roji do you want a test for padleft/right for both with constant and with parameter to check that it doesn't truncate

@ChrisJollyAU ChrisJollyAU linked a pull request Sep 21, 2024 that will close this issue
Open
@roji roji modified the milestones: 9.0.0, 10.0.0 Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
10.0.0
4 participants
@roji @ChrisJollyAU @ozgur-yalcin @Voonti