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

Parsing of datetime to take UTC if timezone is not provided #119

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

silent-lad
Copy link

@silent-lad silent-lad commented May 23, 2024

This is causing problem for us in cases where we have set the trino sql timestamp to be UTC and our database has datetime columns without location information because we assume UTC as default.

In DBs like postgres if not given location is taken as UTC whereas this client parses date to EPOCH with time.Local as the location. WHich gives different result on different machine(cloud/ local laptop)

Copy link

cla-bot bot commented May 23, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented May 23, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@silent-lad
Copy link
Author

Have fixed the tests @nineinchnick

@nineinchnick
Copy link
Member

I've thought about this, and this is a breaking change for anyone using this data type. What's worse, it's silent, meaning the behavior will just change for anyone not pinned to a specific version of this driver. Even if this change is correct, we should only do this in a v2 version.

If you don't mind, I'd keep this open for a while, hoping we can get more feedback from other people. In the meantime, I'd suggest using a workaround, like you can create a struct implementing sql.Scanner in your application and pass it to Scan() instead of trino.Time.

@silent-lad
Copy link
Author

silent-lad commented Jun 18, 2024

Can we have the default timezone passed with the connection string like we have for custom http client. Something like ->

if tzKey := query.Get("default_tz"); tzKey != "" {
     loc, err := time.LoadLocation(tzKey)
     if err == nil {
          time.Local = loc
     }
}

That way its both configurable and backwards compatible

@nineinchnick
Copy link
Member

@cla-bot check

Copy link

cla-bot bot commented Aug 24, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link

cla-bot bot commented Aug 24, 2024

The cla-bot has been summoned, and re-checked this pull request!

@nineinchnick
Copy link
Member

@silent-lad I like your last suggestion, can we get back to this? You haven't signed the CLA yet.

@silent-lad
Copy link
Author

https://github.com/trinodb/cla/pull/8/files
I have a PR for the CLA. Haven't gotten it approved. @nineinchnick

@nineinchnick
Copy link
Member

PRs are not processed in that repository, you need to send the signed CLA via email.

@silent-lad
Copy link
Author

can we get back to this?

I will update the PR with changes today.

you need to send the signed CLA via email.

Is the procedure documented somewhere so I can follow?

@nineinchnick
Copy link
Member

Is the procedure documented somewhere so I can follow?

Yes, please read the cla-bot comments.

@nineinchnick nineinchnick added the breaking-change Includes changes that would require changes in the application using this driver label Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Includes changes that would require changes in the application using this driver
Development

Successfully merging this pull request may close these issues.

2 participants