velox
velox copied to clipboard
Add session timezone to DWRF/ORC TimestampColumnWriter&TimestampColumnReader
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.
Deploy Preview for meta-velox canceled.
| Name | Link |
|---|---|
| Latest commit | ba8c9517656d9a7907d89806018d6f9149d3bb80 |
| Latest deploy log | https://app.netlify.com/sites/meta-velox/deploys/66cd4c361377c100088e1cff |
Hi @Yuhta Any more comments on this PR?
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.
@Yuhta Do I need to split the refactoring of ReaderBase::ReaderBase into a separate PR? Now the API modification also involves nearly 30 files.
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!