Improve round() function return type handling based on input arguments data types #3567

fizzlr · 2025-03-11T06:38:15Z

Description

Make the round() function clearer on return types based on argument types and number of arguments.

Changes:

Add lookup functions for data types that are represented differently
Introduce PLpgSQL functions for each return type to reuse the existing underlying C functions
Implement logic to determine return types based on argument types and number of arguments
Add tests to ensure comprehensive coverage

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.

coveralls · 2025-03-13T22:16:42Z

Pull Request Test Coverage Report for Build 13954105418

Details

30 of 30 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 74.998%

Totals
Change from base Build 13950663165:	0.03%
Covered Lines:	47491
Relevant Lines:	63323

💛 - Coveralls

fizzlr · 2025-03-14T00:17:43Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

-	{"sys", "concat_ws", "concat_ws", true, 1}
+	{"sys", "concat_ws", "concat_ws", true, 1},
+
+	/* Since sql server round() function can take either 2 or 3 parameters, it doesn't fit into case of variable length of paraemters */


here i think is more of including it to tsql_special_function_list that we need to treat them specially.

fizzlr · 2025-03-14T21:21:52Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+		 * is version 12, will follow the existing behavior.
+		 * The existing behavior throws error message if nargs is not from 1 to 3.
+		 */
+		if (nargs == 2 || nargs == 3)


will add a jira to document the behavior and future improvement if it need to

fizzlr · 2025-03-14T00:05:49Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+			|| (new_input_typeids[0] == INT2OID)
+			|| (new_input_typeids[0] == INT4OID))
+			{
+				expr_result_type = (*common_utility_plugin_ptr->lookup_tsql_datatype_oid) ("int");


otherwise data type will be incorrect, since the oid for that in sql server and babelfish are different.

But for input arguments, they always have the same oid as postgres?

What if ROUND(a, 2) while a is a column of babelfish smallint.

Will test that

fizzlr · 2025-03-14T00:06:00Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+			/* bigint will have return type bigint */
+			else if (new_input_typeids[0] == INT8OID)
+			{
+				expr_result_type = (*common_utility_plugin_ptr->lookup_tsql_datatype_oid) ("bigint");


otherwise data type will be incorrect, since the oid for that in sql server and babelfish are different.

hopebo · 2025-03-14T00:06:03Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+			}
+			else
+			{
+				expr_result_type = (*common_utility_plugin_ptr->lookup_tsql_datatype_oid) ("decimal");


Do we have situation hitting the last else branch? From what I see in SQL Server doc, all situations have been covered above.

got it, will check sql server behavior

We do not need this else block, as the return type in other cases are not specified. For other cases lets have the existing behaviour.

removed else block

after removing this else block, it will fail tests from BABEL-1193.out and babel_function.out. Annotated in the code: add else here for backward compatibility and passing old tests

How do the old tests fail? We don't need to keep the backward compatibility. The behaviour of old version is wrong. We only need to keep consistent with SQL Server.

+~~ERROR (Code: 33557097)~~
+
+~~ERROR (Message: function round(text, integer) is not unique)~~

and another one for NULL case is:
+~~ERROR (Code: 33557097)~~
+
+~~ERROR (Message: function round(unknown, integer) is not unique)~~

Chatted about behavior for this, will remove else block and modify old tests accordingly and move them to new test file I added.

fizzlr · 2025-03-17T21:21:44Z

test/JDBC/input/BABEL-2057.sql

+select round(cast(42 as smallmoney), 2)
+go
+
+select round(cast(42 as smallmoney), 999)


added in test

hopebo

Overall, looks good to me. Some minor comments and questions left.

hopebo

Also, please squash your commits into one.

forestkeeper · 2025-03-14T03:31:07Z

test/JDBC/expected/BABEL-2057.out

+int
+~~ERROR (Code: 33557097)~~
+
+~~ERROR (Message: value overflows for numeric format)~~


Is this expected ?

forestkeeper · 2025-03-14T03:31:18Z

test/JDBC/expected/BABEL-2057.out

+go
+~~ERROR (Code: 33557097)~~
+
+~~ERROR (Message: value overflows for numeric format)~~


Is this expected ?

forestkeeper · 2025-03-14T03:31:37Z

test/JDBC/expected/BABEL-2057.out

+go
+~~ERROR (Code: 33557097)~~
+
+~~ERROR (Message: Arithmetic overflow error for data type numeric.)~~


What is this error msg from SQL Server ?

for the overflow message shown in the .out file, sql server files just return 0 or .00

but since those are the existing babelfish behavior, I will leave them as that. If required in future for behavior change, we can revisit this.

forestkeeper · 2025-03-14T03:33:22Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+		{
+			ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("ROUND function expects 2 or 3 arguments")));


Where is the test case for this ?
What's the output of sql server for this ?

will put an assert here instead

Currently babelfish supports round with 1 arguments due to underlying PostgreSQL, if we do not have any good enough reason to block round function call with number of arguments other than 2 or 3, we should not block it. Lets keep the existing behaviour in this case, if we do not have any good enough reason to throw error.

also during my testing, this ereport is unable to be reached. The error about the number of arguments is caught else where already. Remove this ereport

forestkeeper · 2025-03-14T03:34:41Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

@@ -1353,6 +1357,58 @@ tsql_func_select_candidate_for_special_func(List *names, int nargs, Oid *input_t
 			expr_arg_type = (*common_utility_plugin_ptr->lookup_tsql_datatype_oid) ("bbf_varbinary");
 		}
 	}
+	/* The return type of sys.round() will be based on input argument type */
+	else if (strlen(proc_name) == 5 && strncmp(proc_name, "round", 5) == 0)


Case sensitive ?

within this function, following the pattern to use lower case as other special function.

hopebo · 2025-03-14T21:50:44Z

test/JDBC/input/BABEL-2057.sql

+select round(cast(748.58 as float), 2)
+go
+JBA
+select round(cast(748.58 as float), 3)


Also supplement a test case with only one argument and comment on it.

rohit01010 · 2025-03-16T18:02:13Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+			}
+			else
+			{
+				expr_result_type = (*common_utility_plugin_ptr->lookup_tsql_datatype_oid) ("decimal");


We do not need this else block, as the return type in other cases are not specified. For other cases lets have the existing behaviour.

rohit01010 · 2025-03-16T18:04:41Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+		{
+			ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("ROUND function expects 2 or 3 arguments")));


Currently babelfish supports round with 1 arguments due to underlying PostgreSQL, if we do not have any good enough reason to block round function call with number of arguments other than 2 or 3, we should not block it. Lets keep the existing behaviour in this case, if we do not have any good enough reason to throw error.

forestkeeper · 2025-03-18T00:01:42Z

test/JDBC/input/round_return_type_test-vu-cleanup.sql

+GO
+DROP FUNCTION IF EXISTS dbo.RoundFloat;
+GO
+DROP FUNCTION IF EXISTS dbo.RoundDecimal;


This test need to be added to schedule files.

added accordingly

forestkeeper · 2025-03-18T00:04:40Z

test/JDBC/expected/round-return-type.out

+select round(cast(42 as decimal), 0)
+go
+~~START~~
+numeric


I think for decimal, it should return decimal according to this doc :
https://github.com/MicrosoftDocs/sql-docs/blob/live/docs/t-sql/functions/round-transact-sql.md

You right. But after checking the logic in sys_functions.sql and pltsql_coerce.c, I've set them to be decimal already.

In https://learn.microsoft.com/en-us/sql/t-sql/data-types/decimal-and-numeric-transact-sql?view=sql-server-ver16, it says "decimal and numeric are numeric data types that have a fixed precision and scale. decimal and numeric are synonyms and can be used interchangeably."

forestkeeper · 2025-03-18T00:05:13Z

test/JDBC/expected/round-return-type.out

+
+
+
+select round(cast(42 as numeric), 0)


For numeric, it should return decimal according to this doc : https://github.com/MicrosoftDocs/sql-docs/blob/live/docs/t-sql/functions/round-transact-sql.md

Decimal is just an alias of numeric in SQL Server. Not sure if we need to pay more efforts to this.

Correct, but after checking the logic in sys_functions.sql and pltsql_coerce.c, I've set them to be decimal already.

In https://learn.microsoft.com/en-us/sql/t-sql/data-types/decimal-and-numeric-transact-sql?view=sql-server-ver16, it says "decimal and numeric are numeric data types that have a fixed precision and scale. decimal and numeric are synonyms and can be used interchangeably."

…pe and the number of arguments following sql server style Signed-off-by: RUI LUO <[email protected]>

fizzlr · 2025-03-18T21:23:21Z

contrib/babelfishpg_tsql/src/pltsql_coerce.c

+			/* in all other cases it will default to float data type */
+			else
+			{
+				expr_result_type = (*common_utility_plugin_ptr->lookup_tsql_datatype_oid) ("float");


tested in sql server that other data types not mentioned in round sql server documentation will be defaulted to float data type. Matching that behavior here

Good finding! So SQL Server allows other types other than listed in the doc as input, right?
Have you supplemented such test cases?

yes, added to round_return_type_test-vu-verify.sql

…s data types (#3592) Make the round() function clearer on return types based on argument types and number of arguments (#3592) This is a cherry-pick commit from BABEL_4_X_DEV (#3567). Changes: Add lookup functions for data types that are represented differently Introduce PLpgSQL functions for each return type to reuse the existing underlying C functions Implement logic to determine return types based on argument types and number of arguments Add tests to ensure comprehensive coverage Task: BABEL-2057 Signed-off-by: RUI LUO <[email protected]> Co-authored-by: RUI LUO <[email protected]>

fizzlr requested review from rohit01010 and shardgupta March 11, 2025 06:38

fizzlr changed the title ~~Jira babel 2057~~ Make round() function more clear on return types Mar 13, 2025

hopebo reviewed Mar 13, 2025

View reviewed changes

hopebo reviewed Mar 14, 2025

View reviewed changes

forestkeeper reviewed Mar 14, 2025

View reviewed changes

hopebo suggested changes Mar 14, 2025

View reviewed changes

rohit01010 suggested changes Mar 16, 2025

View reviewed changes

hopebo approved these changes Mar 17, 2025

View reviewed changes

forestkeeper reviewed Mar 18, 2025

View reviewed changes

fizzlr force-pushed the jira-babel-2057 branch 2 times, most recently from 734b657 to 35408c1 Compare March 18, 2025 05:00

Make round() function more clear on return types based on argument ty…

7cb777a

…pe and the number of arguments following sql server style Signed-off-by: RUI LUO <[email protected]>

fizzlr force-pushed the jira-babel-2057 branch from 35408c1 to 7cb777a Compare March 18, 2025 05:08

RUI LUO added 3 commits March 18, 2025 05:14

Make round() function more clear on return types based on argument ty…

e2b1a8c

…pe and the number of arguments following sql server style Signed-off-by: RUI LUO <[email protected]>

fix

db3f86f

remove entries from expected_drop.out and expected_dependency.out

0f8a7c8

add fix

bb1c89b

fizzlr changed the title ~~Make round() function more clear on return types~~ Fix issue with round() function on other data types Mar 18, 2025

RUI LUO added 2 commits March 18, 2025 18:55

fix schedule conflict

3c4b17a

fix tests due to behaviors difference

de340f8

fizzlr commented Mar 18, 2025

View reviewed changes

fizzlr changed the title ~~Fix issue with round() function on other data types~~ Improve round() function return type handling per SQL Server rules Mar 18, 2025

fizzlr mentioned this pull request Mar 19, 2025

Improve round() function return type handling based on input arguments data types #3592

Merged

1 task

fizzlr changed the title ~~Improve round() function return type handling per SQL Server rules~~ Improve round() function return type handling based on input arguments data types Mar 19, 2025

resolve conflicts

a034801

forestkeeper approved these changes Mar 19, 2025

View reviewed changes

forestkeeper merged commit d4b2ca5 into babelfish-for-postgresql:BABEL_4_X_DEV Mar 19, 2025
48 checks passed

Improve round() function return type handling based on input arguments data types #3567

Improve round() function return type handling based on input arguments data types #3567

Conversation

fizzlr commented Mar 11, 2025 • edited Loading

Description

Check List

coveralls commented Mar 13, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13954105418

Details

💛 - Coveralls

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

hopebo left a comment

Choose a reason for hiding this comment

hopebo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fizzlr commented Mar 11, 2025 •

edited

Loading

coveralls commented Mar 13, 2025 •

edited

Loading