velox icon indicating copy to clipboard operation
velox copied to clipboard

Add session timezone to DWRF/ORC TimestampColumnWriter&TimestampColumnReader

Open wypb opened this issue 1 year ago • 1 comments

As described in https://github.com/facebookincubator/velox/issues/8127, Velox's DWRF/ORC TimestampColumnReader and TimestampColumnWriter currently hard-code the time zone. If we use Presto to write timestamps and use velox to read them, the results will be incorrect, and vice versa.

This PR passes ConnectorQueryCtx#sessionTimezone() to TimestampColumnReader and TimestampColumnWriter, allowing us to use the user-set Timezone.

wypb avatar Jul 02 '24 11:07 wypb

Deploy Preview for meta-velox canceled.

Name Link
Latest commit ba8c9517656d9a7907d89806018d6f9149d3bb80
Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66cd4c361377c100088e1cff

netlify[bot] avatar Jul 02 '24 11:07 netlify[bot]

Hi @Yuhta Any more comments on this PR?

wypb avatar Aug 14 '24 09:08 wypb

On last minor thing and we are almost done. The change becomes quite large though, would you split it into 2 PRs, one for refactor (adding all the fields/methods needed but no functionality change at all) and the other that actually uses sessionTimezone_ and adjustTimestampToTimezone_ in column reader/writer?

Sure, I'll split it into two PRs. Thank you for your reply.

wypb avatar Aug 30 '24 01:08 wypb

@Yuhta Do I need to split the refactoring of ReaderBase::ReaderBase into a separate PR? Now the API modification also involves nearly 30 files.

wypb avatar Aug 30 '24 02:08 wypb

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

stale[bot] avatar Nov 30 '24 17:11 stale[bot]