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.

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

Fix local timezone #550

Merged
merged 3 commits into from
Apr 28, 2024
Merged

Fix local timezone #550

merged 3 commits into from
Apr 28, 2024

Conversation

rocksnow1942
Copy link
Contributor

@rocksnow1942 rocksnow1942 commented Apr 26, 2024

A small followup to fix #546

@sijms
Copy link
Owner

sijms commented Apr 26, 2024

can you explain

case DATE, TIMESTAMP, TIMESTAMP_DTY:
    par.oPrimValue = tempTime.In(conn.dbTimeZone)

all this type store datetime without timezone information. so you now returning the value in timezone?
the original code return the value as it is and just append dbTimeZone (suggested by someone).
before I just leave timezone information empty (default is UTC)

@rocksnow1942
Copy link
Contributor Author

rocksnow1942 commented Apr 27, 2024

Thanks for pointing out. Yes, you are right, I thought about it more, and changed to:

	par.oPrimValue = time.Date(tempTime.Year(), tempTime.Month(), tempTime.Day(),
		tempTime.Hour(), tempTime.Minute(), tempTime.Second(), tempTime.Nanosecond(), time.Local)

This way, the DATE, TIMESTAMP, TimeStampDTY are set to the system time zone. This is the same behavior as before commit bbeff0e.

Here is why:

Before commit bbeff0e, getDBTimeZone query SELECT SYSTIMESTAMP FROM DUAL to get conn.dbTimeLoc. "SYSTIMESTAMP" uses system timezone.
(see oracle doc here)

SYSTIMESTAMP returns the system date, including fractional seconds and time zone, of the system on which the database resides. The return type is TIMESTAMP WITH TIME ZONE.

So in v2/parameter.go, under case DATE, TIMESTAMP, TimeStampDTY, the time stamp appended with system timezone (conn.dbTimeLoc).

After commit bbeff0e, that is no longer the case. conn.dbTimeZone is the DBTIMEZONE, which can be different from system timezone. So this PR changes to append the time with time.Local, which is the system local time zone.

@sijms
Copy link
Owner

sijms commented Apr 28, 2024

ok time.Local will use client timezone so better to return the previous variable dbTimeZone in new name dbServerTimeZone and restore its function

@sijms
Copy link
Owner

sijms commented Apr 28, 2024

I make these changes in last commit

@sijms
Copy link
Owner

sijms commented Apr 28, 2024

database server timezone +03:00
database timezone: +00:00
the result of examplels/time_issue

DATE:                     2024-04-28 06:27:23 +0300 +03:00
Timestamp:                2024-04-28 06:27:23.9229 +0300 +03:00
Timestamp TZ:             2024-04-28 11:27:23.9229 +0800 CST
Timestamp with local TZ:  2024-04-28 03:27:23.922901 +0000 +00:00

this confirmed by

select SYSTIMESTAMP AT TIME ZONE DBTIMEZONE from dual;

the result: 28/04/24 03:22:23.563000000 AM GMT from sql developer

@rocksnow1942
Copy link
Contributor Author

Thanks, will you make a new release anytime soon? I can test your new version.

@rocksnow1942
Copy link
Contributor Author

rocksnow1942 commented Apr 28, 2024

I noticed in your latest commit you didn't fix TimeStampeLTZ, I think the following is still needed.

case TimeStampeLTZ, TimeStampLTZ_DTY:
	tempTime, err := converters.DecodeDate(par.BValue)
	if err != nil {
		return err
	}
	par.oPrimValue = tempTime
	if conn.dbTimeZone != time.UTC {
		par.oPrimValue = time.Date(tempTime.Year(), tempTime.Month(), tempTime.Day(),
			tempTime.Hour(), tempTime.Minute(), tempTime.Second(), tempTime.Nanosecond(), conn.dbTimeZone)
	}

Your test result look correct because the database timezone is +00:00.

@sijms sijms merged commit b676477 into sijms:master Apr 28, 2024
@rocksnow1942 rocksnow1942 deleted the rocksnow1942/fix-LTZ branch April 29, 2024 01:18
@rocksnow1942
Copy link
Contributor Author

Thanks! When should I expect a new release?

@sijms
Copy link
Owner

sijms commented Apr 30, 2024

within days. I will fix other issues and make the release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timestamp with Local Time Zone conversion
2 participants