fix for char and nchar function correct handling by pranavJ23 · Pull Request #3855 · babelfish-for-postgresql/babelfish_extensions

pranavJ23 · 2025-06-19T12:44:25Z

Description

Function char throws error if the input integer is not in range 0-255, where as tsql returns NULL instead of error.
Function nchar should throw NULL if the the input integer is not in range 0-65535
Fix for nchar() function so that if input type is coercible to int then it should execute successfully without throwing error

Issues Resolved

[BABEL-4810] [BABEL-4812][BABEL-5933]

Signed-off-by: Pranav Jain pranavjc@amazon.com

Test Scenarios Covered

Use case based -
Boundary conditions -
Arbitrary inputs -
Negative test cases -
Minor version upgrade tests -
Major version upgrade tests -
Performance tests -
Tooling impact -
Client tests -

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

coveralls · 2025-06-20T19:15:55Z

Pull Request Test Coverage Report for Build 15966586600

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

23 of 23 (100.0%) changed or added relevant lines in 1 file are covered.
35 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.04%) to 75.872%

Files with Coverage Reduction	New Missed Lines	%
contrib/babelfishpg_tsql/src/cursor.c	35	84.53%

Totals
Change from base Build 15889588684:	0.04%
Covered Lines:	49471
Relevant Lines:	65203

💛 - Coveralls

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Anikait143

Please add following test case:

Tests with schema.function_call
Test with different collation settings
Test for multibyte character input integer value.
Test with edge case values
Test with datatype inputs that can be implicitly casted to int

Anikait143 · 2025-06-22T18:49:59Z

contrib/babelfishpg_common/sql/upgrades/babelfish_common_helper--5.2.0--5.3.0.sql

    END IF;
 END $$;

+create or replace function sys.cht(x in int) returns sys.varchar


Old function should be deprecated since we have changed function signature

Anikait143 · 2025-06-22T18:52:09Z

contrib/babelfishpg_common/sql/string_operators.sql

 END;
 $body$
-language plpgsql STABLE;
+LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;


We should mark all the functions as STABLE since the underlying helper function depends on encoding settings

with stable we are not able to use the CHAR function in computed column, where as in tsql the char can be used in computed column.
Since the encoding in babelfish_db can' t be changed once the db is formed, hence we can mark the function as immutable

Anikait143 · 2025-06-22T18:54:22Z

contrib/babelfishpg_common/sql/string_operators.sql

 END;
 $body$
-language plpgsql STABLE;
+LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;


Have we verified that it is really PARALLEL SAFE?

Since the test case is passing in the JDBC parallel query mode, so we can say that it's parallel safe ?

From the pg_proc catalog too, we have verified that chr() function is parallel safe and since we are using chr() underlying sys.char(), so we can say it's parallel safe

Anikait143 · 2025-06-22T19:04:27Z

contrib/babelfishpg_common/sql/string_operators.sql

 );

-create or replace function sys.CHAR(x in int)returns char
+create or replace function sys.cht(x in int) returns sys.varchar


Return type should be sys.bpchar

IMO varchar might be a better return function since bpchar using fixed length whereas the output will be of length = 1 always, so it might allocate extra space in case of bpchar. Else anything can be used, not an issue.

Anikait143 · 2025-06-22T19:06:26Z

contrib/babelfishpg_common/sql/string_operators.sql

+create or replace function sys.cht(x in int) returns sys.varchar
 AS
 $body$
 BEGIN


What if input is 0? (Same for other functions as well)

yes, this edge case need to be handled since pg function 'chr' expects the input b/w 1-255 where as the tsql expects input b/w 0-255. Thanks

Anikait143 · 2025-06-22T19:35:43Z

contrib/babelfishpg_tsql/src/tsqlIface.cpp

+		if (proc->DOUBLE_QUOTE_ID())
+			stream.setText(startIndex, "\"cht\" ");
+		else if (proc->SQUARE_BRACKET_ID())
+			stream.setText(startIndex, "[cht] ");		
+		else
+			stream.setText(startIndex, " cht");


Would there be any case where DOUBLE_QUOTE_ID and SQUARE_BRACKET_ID are valid for function calls?

Yes we do support the query like :

select [char](255)

adding tests for it

Anikait143 · 2025-06-22T19:37:47Z

contrib/babelfishpg_tsql/src/tsqlIface.cpp

+		else if (proc->SQUARE_BRACKET_ID())
+		        stream.setText(startIndex, "[cht] ");		
+		else
+		        stream.setText(startIndex, " cht");


It's better to have proper text replacement maybe use underscore instead of space for new function name.

Anikait143 · 2025-06-22T19:48:31Z

contrib/babelfishpg_common/sql/string_operators.sql

 AS
 $body$
 BEGIN
    --- 1114111 is 0x10FFFF - max value permitted as specified by documentation


Let's also correct the comment here

Should this function be dependant upon the input collation ?

contrib/babelfishpg_common/sql/string_operators.sql

Anikait143 · 2025-06-22T19:50:41Z

contrib/babelfishpg_common/sql/string_operators.sql

-language plpgsql STABLE;
+LANGUAGE plpgsql IMMUTABLE STRICT PARALLEL SAFE;

 CREATE OR REPLACE FUNCTION sys.nchar(IN x INTEGER) RETURNS sys.nvarchar


Return type should be nchar

IMO nvarchar might be a better return function since nchar using fixed length whereas the output will be of length = 1 always, so it might allocate extra space in case of nchar. Else anything can be used, not an issue.

tanscorpio7 · 2025-06-23T13:43:16Z

contrib/babelfishpg_common/sql/string_operators.sql

 AS
 $body$
 BEGIN
    --- 1114111 is 0x10FFFF - max value permitted as specified by documentation


Should this function be dependant upon the input collation ?

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Anikait143 · 2025-06-26T16:47:22Z

contrib/babelfishpg_common/sql/upgrades/babelfish_common_helper--5.2.0--5.3.0.sql

+DECLARE
+    exception_message text;
+BEGIN
+    ALTER FUNCTION sys.char(x in int) RENAME TO chr_deprecated_5_2_0;


Deprecated version 5.3.0, please correct the name everywhere.

Anikait143 · 2025-06-26T16:55:31Z

contrib/babelfishpg_common/sql/upgrades/babelfish_common_helper--5.2.0--5.3.0.sql

+DECLARE
+    exception_message text;
+BEGIN
+   ALTER FUNCTION sys.nchar(In x INTEGER) RENAME TO nchar_int_deprecated_5_2_0;


We should also drop the deprecated functions. Refer:

babelfish_extensions/contrib/babelfishpg_common/sql/upgrades/spatial_types--5.2.0--5.3.0.sql

Line 55 in e6e094c

CALL sys.babelfish_drop_deprecated_object('function', 'sys', 'stgeomfromtext_helper_deprecated_5_3_0');

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

… char & nchar function handling Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

tanscorpio7 · 2025-06-29T17:15:19Z

contrib/babelfishpg_tsql/src/tsqlIface.cpp

+	if (pg_strcasecmp(procNameStr.c_str(), "nchar") ==  0)
+	{
+		int startIndex = proc->start->getStartIndex();
+		std::string schNameStr;
+
+		if (schema)
+			schNameStr = stripQuoteFromId(schema);
+
+		if(schema && !schNameStr.empty() && pg_strcasecmp(schNameStr.c_str(), "sys") != 0)
+			return;
+
+		if (proc->DOUBLE_QUOTE_ID())
+			stream.setText(startIndex, "\"ncht_\"");
+		else if (proc->SQUARE_BRACKET_ID())
+			stream.setText(startIndex, "[ncht_]");		
+		else
+			stream.setText(startIndex, "ncht_");


Looks like this block is duplicated 4 times. Can we create one helper function for this ?

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

…resql#3855) Function char throws error if the input integer is not in range 0-255, where as tsql returns NULL instead of error. Function nchar should throw NULL if the the input integer is not in range 0-65535 Fix for nchar() function so that if input type is coercible to int then it should execute successfully without throwing error Task: [BABEL-4810] [BABEL-4812][BABEL-5933] Signed-off-by: pranav jain <pranav23iitd@gmail.com>

pranavJ23 and others added 7 commits June 19, 2025 12:42

fix for char and nchar function correct handling

027e0dc

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Merge remote-tracking branch 'upstream/BABEL_5_X_DEV' into babel-4810

11ac3a8

Merge remote-tracking branch 'upstream/BABEL_5_X_DEV' into babel-4810

8fa8985

fixing expected files

295dddc

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

fixing the test collation output

f04f307

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Merge branch 'BABEL_5_X_DEV' into babel-4810

00dbe04

Merge remote-tracking branch 'upstream/BABEL_5_X_DEV' into babel-4810

2c9ae0b

pranavJ23 added 3 commits June 22, 2025 12:47

Merge remote-tracking branch 'origin/babel-4810' into babel-4810

b14d538

updating upgrade files

4b1b6b5

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

minor changes

f28916d

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Anikait143 suggested changes Jun 22, 2025

View reviewed changes

tanscorpio7 requested changes Jun 23, 2025

View reviewed changes

pranavJ23 added 3 commits June 25, 2025 10:11

fix for nchar when input is coercible to int type

b6793b7

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

minor fixes

3f8c48d

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

minor fixes

d190bc7

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

pranavJ23 requested review from Anikait143 and tanscorpio7 June 25, 2025 16:59

pranavJ23 and others added 2 commits June 26, 2025 13:32

Merge branch 'babelfish-for-postgresql:BABEL_5_X_DEV' into babel-4810

f290dae

fixing datetime changes

ed7b626

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Anikait143 reviewed Jun 26, 2025

View reviewed changes

pranavJ23 added 6 commits June 27, 2025 05:29

addressing comments

57a2436

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

minor upgrade fixes

e6a5113

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

expected_drop change

339c8a9

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

fixing edge case, adding test with quoted identifier, simplifying the…

e1e35eb

… char & nchar function handling Signed-off-by: pranav jain <pranav23iitd@gmail.com>

fixing expected test files

25b99f8

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

fixing test

58183b8

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

tanscorpio7 requested changes Jun 29, 2025

View reviewed changes

pranavJ23 added 2 commits June 30, 2025 06:16

adding helper function and more tests

cb7533b

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

fixing test updates collation

e249f6f

Signed-off-by: pranav jain <pranav23iitd@gmail.com>

Anikait143 approved these changes Jun 30, 2025

View reviewed changes

pranavJ23 requested a review from tanscorpio7 June 30, 2025 15:29

tanscorpio7 approved these changes Jun 30, 2025

View reviewed changes

Merge branch 'babelfish-for-postgresql:BABEL_5_X_DEV' into babel-4810

0cd0a3d

shardgupta approved these changes Jul 1, 2025

View reviewed changes

shardgupta merged commit 36085f6 into babelfish-for-postgresql:BABEL_5_X_DEV Jul 1, 2025
52 of 55 checks passed

Conversation

pranavJ23 commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Test Scenarios Covered

Check List

Uh oh!

coveralls commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 15966586600

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Anikait143 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavJ23 Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anikait143 Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pranavJ23 commented Jun 19, 2025 •

edited

Loading

coveralls commented Jun 20, 2025 •

edited

Loading

pranavJ23 Jun 23, 2025 •

edited

Loading

Anikait143 Jun 22, 2025 •

edited

Loading