datepart function reimplementation #1959

ParikshitSarode · 2023-10-26T08:45:39Z

Description - Low performance with function sys. datepart_internal -- re-implement in `C`

sys. datepart_internal function takes longer time in babelfish then expected
The function is reimplemented in C for faster execution.
Different wrapper functions are created according to the data type of the second argument in sys. datepart_internal.
Additional tests are added covering all cases since previously incorrect query outputs were written in tests and logical errors were present in code

Queries with incorrect output that were present in the JDBC tests files

In datepart, some queries in tests were giving incorrect output and included in the tests file. For example,

Example 1

SELECT DATEPART(ms,'2016-12-26 23:30:05.523456');
GO
~~START~~       
int
456
~~END~~

The correct output should have been 523 which is verified and fixed in this code. This incorrect test was present in test/JDBC/expected/babel_function.out among others

Example 2

SELECT DATEPART(ss, '12-31-9999 23:59:59.999999 +14:00')
GO
~~START~~
int
0
~~END~~

The expected result should be 59

Example 3 -

SELECT DATEPART(hh, 'Jul 18, 22 01:01:00.1234567 -7:8')
GO
~~START~~
int
8
~~END~~

Correct output should be 1 but it was written as 8 in test/JDBC/expected/datepart-vu-verify.out

When verified, the output of this query should be 1

Such outputs are corrected in the [BABEL-4302] - Low performance with function sys. datepart_internal -- re-implement in C. All queries are giving correct results as expected.

The file test/JDBC/expected/babel_function.out has the following queries that should throw an error but some output is given

datepart queries should throw an error message for the following queries instead some output is given. These queries are present in the babel_functions.out in test/JDBC/expected/babel_function.out

Example 1-
BBF results

select datepart(month, cast('12:10:30.123' as time));
GO
~START~
int
1
~END~

Expected results

select datepart(month, cast('12:10:30.123' as time));
go
Msg 9810, Level 16, State 3, Server EC2AMAZ-JA3CRFV, Line 1
The datepart month is not supported by date function datepart for data type time.

When the first parameter in datepart is date, week, month, quarter, year, such error is expected.

Example 2-
BBF results

select datepart(q, CAST('2016-12-26 23:30:05.523456+8'AS datetimeoffset));
GO
~~START~~
int
4
~~END~~

Expected results

select datepart(q, CAST('2016-12-26 23:30:05.523456+8'AS datetimeoffset));
go
Msg 241, Level 16, State 1, Server EC2AMAZ-JA3CRFV, Line 1
Conversion failed when converting date and/or time from character string.

Irrespective of the first parameter in datepart, such error is expected.
These are some few examples of incorrect behaviour

Signed-off-by: Parikshit Sarode [email protected]

Issues Resolved

Task: BABEL-4302,BABEL-979, BABEL-4366 issues related to datepart, BABEL-4582 issues related to datepart and not cast

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.

contrib/babelfishpg_tsql/runtime/functions.c

Deepesh125 · 2023-12-28T10:59:06Z

contrib/babelfishpg_tsql/runtime/functions.c

+#define DATEPART_MAX_VALUE 2958463
+#define DATEPART_MIN_VALUE -53690


what does these two number represent? add simple comment just like above

Deepesh125 · 2023-12-28T11:00:25Z

contrib/babelfishpg_tsql/sql/upgrades/babelfishpg_tsql--3.4.0--3.5.0.sql

+ALTER FUNCTION sys.datepart_internal RENAME TO datepart_internal_deprecated_3_5;
+
+CALL sys.babelfish_drop_deprecated_object('function', 'sys', 'datepart_internal_deprecated_3_5');


Have defensive code, rename only if it exists with given argument types.

You can take help from Rohit

Deepesh125 · 2023-12-28T11:04:41Z

contrib/babelfishpg_tsql/runtime/functions.c

+Datum
+datepart_internal_datetimeoffset(PG_FUNCTION_ARGS)
+{
+	char		*field = text_to_cstring(PG_GETARG_TEXT_PP(0));


You can directly use PG_GETARG_CSTRING, applicable to everywhere else also

This is not yet resolved

Deepesh125 · 2023-12-28T11:17:17Z

contrib/babelfishpg_tsql/runtime/functions.c

+	DateADT		date_arg;
+	date_arg = PG_GETARG_DATEADT(1);
+	timestamp = DirectFunctionCall1(date_timestamp, date_arg);


DirectFunctionCall1 expects datum argument so we can directly pass PG_GETARG(1) to DirectFunctionCall1

Deepesh125 · 2023-12-28T11:58:18Z

contrib/babelfishpg_tsql/runtime/functions.c

+	if (timestamp == 0 && general_integer_datatype)
+	{	
+		/* Checking for the limits for general_integer_datatype */
+		if(df_tz > DATEPART_MAX_VALUE || df_tz < DATEPART_MIN_VALUE)
+		{
+			ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("Arithmetic overflow error converting expression to data type datetime.")));
+		}


add test cases which will hit this error

Deepesh125 · 2023-12-29T11:03:47Z

contrib/babelfishpg_tsql/runtime/functions.c

+Datum
+datepart_internal_datetimeoffset(PG_FUNCTION_ARGS)
+{
+	char		*field = text_to_cstring(PG_GETARG_TEXT_PP(0));


This is not yet resolved

Deepesh125 · 2023-12-29T11:13:47Z

contrib/babelfishpg_tsql/sql/upgrades/babelfishpg_tsql--3.4.0--3.5.0.sql

+
+    ALTER FUNCTION sys.datepart_internal(PG_CATALOG.TEXT, anyelement, INTEGER) RENAME TO datepart_internal_deprecated_3_5;
+
+    CALL sys.babelfish_drop_deprecated_object('function', 'sys', 'datepart_internal_deprecated_3_5');


should be done at the end of script

Deepesh125 · 2023-12-29T11:20:35Z

test/JDBC/input/BABEL_4302.sql

+SELECT DATEPART(day, CAST(-53690 AS float));
+GO
+
+SELECT DATEPART(day, CAST(-53691 AS float));


we still need to have test cases which covers min/max, NULL for all the data types and also with random field argument

Deepesh125 · 2024-01-02T04:01:19Z

test/JDBC/expected/BABEL_4302.out

+SELECT DATEPART(second, CAST(256 AS tinyint));
+GO
+~~START~~
+int
+~~ERROR (Code: 220)~~
+
+~~ERROR (Message: value for domain tinyint violates check constraint "tinyint_check")~~


Is this the same T-SQL behaviour? I dont think so. Seems like some bug with casting 256 to TINYINT. Please confirm and file a bug if needed

seems like every domain data type has this problem, for example, smallmoney

This is a issue of cast function. For the datatypes, the overflow error message is thrown from the cast function before hitting the datepart functions. Will fiile a bug

PR implements fix for low performance with function sys. datepart_internal by re-implement in C Signed-off-by: Parikshit Sarode <[email protected]> Co-authored-by: Parikshit Sarode <[email protected]>

datepart_internal reimplemented

22bb48f

ParikshitSarode requested review from KushaalShroff, Deepesh125 and forestkeeper October 26, 2023 08:47

Parikshit Sarode added 18 commits October 26, 2023 08:51

datepart_internal fixed

9e65c17

nanosec condition updated

9c401fa

datetime accepted

a82dd20

error message fixed

d759465

date condition added

45350c3

datetime2 accepted

78130df

wrapper functions created

2445cba

Merge branch 'BABEL_3_X_DEV' into Jira-4302

0bbb10f

nanosec modified

e609894

unique fixed

afe5e49

unique fixed

83e6f63

changes reverted

e88fa5c

offset adjusted for dow

5802141

all datatypes covered

5d59468

money fixed

13953bd

error message fixed

0b97c25

tests added

0ae14d0

tests added

433951a

ParikshitSarode removed the request for review from forestkeeper November 9, 2023 07:47

KushaalShroff requested changes Nov 9, 2023

View reviewed changes

contrib/babelfishpg_tsql/runtime/functions.c Show resolved Hide resolved

contrib/babelfishpg_tsql/runtime/functions.c Show resolved Hide resolved

Parikshit Sarode added 6 commits November 9, 2023 13:48

week implementation changed

cde18b5

Empty-Commit

a58e1ba

conflict resolved

ef162a5

week and tsql_week reimplemented

487903c

jdbc_schedule mofified

ae0d645

comment resolved

cd24a14

Deepesh125 requested changes Dec 28, 2023

View reviewed changes

Parikshit Sarode added 8 commits December 28, 2023 14:15

comments resolved

c096423

comments resolved

efdd266

comments resolved

9f1ef7d

Empty-Commit

640ec43

recreating failure

9c24cef

changes reverted

f90c043

comment resolved

682318e

changes reverted

524054a

Deepesh125 requested changes Dec 29, 2023

View reviewed changes

comments fixed

c589883

Deepesh125 reviewed Jan 2, 2024

View reviewed changes

Parikshit Sarode added 2 commits January 2, 2024 07:34

tests added

cb24877

Empty-Commit

e14002e

KushaalShroff changed the title ~~WIP - datepart_internal reimplemented~~ datepart function reimplementation Jan 2, 2024

KushaalShroff approved these changes Jan 2, 2024

View reviewed changes

Deepesh125 approved these changes Jan 2, 2024

View reviewed changes

jsudrik merged commit 5c67087 into babelfish-for-postgresql:BABEL_3_X_DEV Jan 2, 2024
30 checks passed

Anikait143 deleted the Jira-4302 branch February 27, 2024 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datepart function reimplementation #1959

datepart function reimplementation #1959

ParikshitSarode commented Oct 26, 2023 •

edited

Loading

Deepesh125 Dec 28, 2023

Deepesh125 Dec 28, 2023

Deepesh125 Dec 28, 2023

Deepesh125 Dec 28, 2023

Deepesh125 Dec 29, 2023

Deepesh125 Dec 28, 2023

Deepesh125 Dec 28, 2023

Deepesh125 Dec 29, 2023

Deepesh125 Dec 29, 2023

Deepesh125 Dec 29, 2023

Deepesh125 Jan 2, 2024

Deepesh125 Jan 2, 2024

ParikshitSarode Jan 2, 2024

		#define DATEPART_MAX_VALUE 2958463
		#define DATEPART_MIN_VALUE -53690

		ALTER FUNCTION sys.datepart_internal RENAME TO datepart_internal_deprecated_3_5;

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


		ALTER FUNCTION sys.datepart_internal(PG_CATALOG.TEXT, anyelement, INTEGER) RENAME TO datepart_internal_deprecated_3_5;

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

datepart function reimplementation #1959

datepart function reimplementation #1959

Conversation

ParikshitSarode commented Oct 26, 2023 • edited Loading

Description - Low performance with function sys. datepart_internal -- re-implement in C

Queries with incorrect output that were present in the JDBC tests files

Issues Resolved

Check List

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

ParikshitSarode commented Oct 26, 2023 •

edited

Loading

Description - Low performance with function sys. datepart_internal -- re-implement in `C`