Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and writer options #10895

wypb · 2024-08-30T03:19:31Z

As suggested here This PR refactors the DRWF Reader and Writer related APIs so that we can get sessionTimezone and adjustTimestampToTimezone in DRWF's ColumnWriter and ColumnReader. No functionality change.

netlify · 2024-08-30T03:19:49Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`11541d0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/670742c8405e8c00081ad7a7

velox/connectors/hive/SplitReader.cpp

velox/dwio/common/tests/utils/E2EFilterTestBase.h

wypb · 2024-09-06T09:32:23Z

@Yuhta The new commit resolves the conflict with the main branch. Please review it again if necessary.

mbasmanova · 2024-09-09T09:41:20Z

@wypb Would you rebase this PR so we can merge it?

facebook-github-bot · 2024-09-09T11:03:54Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

wypb · 2024-09-09T11:04:00Z

@wypb Would you rebase this PR so we can merge it?

@mbasmanova I have synced the latest code, thanks.

facebook-github-bot · 2024-09-09T11:14:23Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Yuhta · 2024-09-09T13:30:15Z

@wypb Can you rebase to the latest main so we can merge it?

wypb · 2024-09-09T14:32:02Z

Hi @Yuhta I have synced the latest code.

velox/dwio/dwrf/reader/ReaderBase.cpp

Yuhta · 2024-09-20T18:28:10Z

velox/dwio/dwrf/reader/ReaderBase.h


  /// Creates reader base from metadata.
  ReaderBase(
-      memory::MemoryPool& pool,
+      const dwio::common::ReaderOptions& options,


This signature is used in other integration inside Meta so cannot be changed

Hi @Yuhta I have rolled back this part of the code.

There is a CI that turns red and is related to this

facebook-github-bot · 2024-09-24T20:48:06Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-09-24T22:41:56Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Yuhta · 2024-09-25T16:06:28Z

velox/dwio/dwrf/reader/ReaderBase.h

-      memory::MemoryPool& pool,
-      std::unique_ptr<dwio::common::BufferedInput> input,
-      dwio::common::FileFormat fileFormat);
+      const dwio::common::ReaderOptions& options,


@wypb We also need another one for ReaderBase(memory::MemoryPool&, std::unique_ptr<dwio::common::BufferedInput>) (and possibly the following default arguments, but let's create this one first)

Hi @Yuhta I have rolled back this part of the code. Please help me review it, thank you.

We are missing one for

ReaderBase( memory::MemoryPool& pool, std::unique_ptr<dwio::common::BufferedInput> input)

Could be just adding a default argument for fileFormat in the signature below.

Hi @Yuhta Sorry, I misunderstood, I have already given a default value to the fileFormat parameter. Please help me review it, thank you.

facebook-github-bot · 2024-10-02T14:58:38Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-10-08T16:04:20Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@wypb thanks for change % nits.

xiaoxmeng · 2024-10-09T04:09:32Z

velox/connectors/Connector.h

+  /// Whether to adjust Timestamp to the timeZone obtained through
+  /// sessionTimezone(). This is used to be compatible with the
+  /// old logic of Presto.
+  const bool adjustTimestampToTimezone() const {


xiaoxmeng · 2024-10-09T04:09:39Z

velox/connectors/Connector.h

@@ -378,6 +387,7 @@ class ConnectorQueryCtx {
  const int driverId_;
  const std::string planNodeId_;
  const std::string sessionTimezone_;
+  bool adjustTimestampToTimezone_;


add const here

xiaoxmeng · 2024-10-09T04:10:10Z

velox/connectors/hive/HiveDataSink.cpp

@@ -744,6 +744,13 @@ uint32_t HiveDataSink::appendWriter(const HiveWriterId& id) {
      connectorSessionProperties,
      options);

+  const auto& sessionTzName = connectorQueryCtx_->sessionTimezone();


s/sessionTzName/sessionTimeZoneName/

xiaoxmeng · 2024-10-09T04:11:53Z

velox/dwio/dwrf/reader/ReaderBase.h

+    return options;
+  }
+
+  const dwio::common::ReaderOptions options_;


Can you move const member first? Thanks!

xiaoxmeng · 2024-10-09T04:12:23Z

velox/dwio/dwrf/reader/StripeStream.h

@@ -103,6 +103,18 @@ class StripeStreams {
   */
  virtual const dwio::common::ColumnSelector& getColumnSelector() const = 0;

+  /**


Can you use /// for comments to be consistent with other code places? Thanks!

wypb · 2024-10-09T11:48:17Z

Hi @xiaoxmeng Addressed all the comments, can you help review again? Thanks!

Yuhta · 2024-10-09T20:33:43Z

@wypb There are some build errors

…writer options

wypb · 2024-10-10T06:36:11Z

@wypb There are some build errors

@Yuhta I've already fixed it, thank you for your reply.

facebook-github-bot · 2024-10-10T13:48:10Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-10-10T17:16:20Z

@Yuhta merged this pull request in 900e61c.

conbench-facebook · 2024-10-10T17:57:50Z

Conbench analyzed the 1 benchmark run on commit 900e61c4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

…writer options (facebookincubator#10895) Summary: As suggested [here](facebookincubator#10372 (review)) This PR refactors the DRWF Reader and Writer related APIs so that we can get `sessionTimezone` and `adjustTimestampToTimezone` in DRWF's ColumnWriter and ColumnReader. No functionality change. Pull Request resolved: facebookincubator#10895 Reviewed By: xiaoxmeng Differential Revision: D62375865 Pulled By: Yuhta fbshipit-source-id: e4f2f7e0383179aff0e87012f04f07d3119b2d32

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2024

wypb force-pushed the refactor_dwrf_api branch 3 times, most recently from d8e0416 to bb946f5 Compare August 30, 2024 05:08

Yuhta changed the title ~~Refactor DWRF read and write related APIs~~ Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and writer options Aug 30, 2024

Yuhta reviewed Aug 30, 2024

View reviewed changes

velox/connectors/hive/SplitReader.cpp Outdated Show resolved Hide resolved

velox/dwio/common/tests/utils/E2EFilterTestBase.h Outdated Show resolved Hide resolved

wypb force-pushed the refactor_dwrf_api branch 4 times, most recently from e9ff5b5 to 331b771 Compare September 3, 2024 03:30

Yuhta approved these changes Sep 5, 2024

View reviewed changes

Yuhta added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 5, 2024

wypb force-pushed the refactor_dwrf_api branch from 9d732ac to e0e82bd Compare September 6, 2024 09:31

wypb force-pushed the refactor_dwrf_api branch 3 times, most recently from f7dda0a to b356a66 Compare September 9, 2024 10:57

wypb force-pushed the refactor_dwrf_api branch from 566dba1 to fe628ec Compare September 9, 2024 11:11

Yuhta reviewed Sep 9, 2024

View reviewed changes

velox/dwio/dwrf/reader/ReaderBase.cpp Outdated Show resolved Hide resolved

wypb force-pushed the refactor_dwrf_api branch 4 times, most recently from 04a1e1f to 029a035 Compare September 10, 2024 06:31

Yuhta reviewed Sep 20, 2024

View reviewed changes

wypb force-pushed the refactor_dwrf_api branch 2 times, most recently from 65e360e to 2813162 Compare September 24, 2024 02:08

Yuhta reviewed Sep 25, 2024

View reviewed changes

wypb force-pushed the refactor_dwrf_api branch 2 times, most recently from 05a1673 to 4112cf3 Compare September 27, 2024 02:56

wypb force-pushed the refactor_dwrf_api branch from 1621915 to 483d36e Compare October 8, 2024 08:17

xiaoxmeng reviewed Oct 9, 2024

View reviewed changes

wypb requested a review from majetideepak as a code owner October 9, 2024 11:46

wypb force-pushed the refactor_dwrf_api branch from 33f25c4 to 37146b6 Compare October 9, 2024 11:46

wypb force-pushed the refactor_dwrf_api branch 2 times, most recently from 5735935 to 14dda78 Compare October 10, 2024 01:35

Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and …

11541d0

…writer options

wypb force-pushed the refactor_dwrf_api branch from 14dda78 to 11541d0 Compare October 10, 2024 02:58

facebook-github-bot closed this in 900e61c Oct 10, 2024

facebook-github-bot added the Merged label Oct 10, 2024

wypb deleted the refactor_dwrf_api branch October 11, 2024 00:07

wypb mentioned this pull request Oct 11, 2024

fix: Add supports time zone adjustment when reading and writing Timestamp data #11232

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and writer options #10895

Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and writer options #10895

wypb commented Aug 30, 2024 •

edited by Yuhta

Loading

netlify bot commented Aug 30, 2024 •

edited

Loading

wypb commented Sep 6, 2024

mbasmanova commented Sep 9, 2024

facebook-github-bot commented Sep 9, 2024

wypb commented Sep 9, 2024 •

edited

Loading

facebook-github-bot commented Sep 9, 2024

Yuhta commented Sep 9, 2024

wypb commented Sep 9, 2024

Yuhta Sep 20, 2024

wypb Sep 24, 2024 •

edited

Loading

facebook-github-bot commented Sep 24, 2024

facebook-github-bot commented Sep 24, 2024

Yuhta Sep 25, 2024 •

edited

Loading

wypb Sep 27, 2024

Yuhta Oct 2, 2024

wypb Oct 8, 2024

facebook-github-bot commented Oct 2, 2024

facebook-github-bot commented Oct 8, 2024

xiaoxmeng left a comment

xiaoxmeng Oct 9, 2024

xiaoxmeng Oct 9, 2024

xiaoxmeng Oct 9, 2024

xiaoxmeng Oct 9, 2024

xiaoxmeng Oct 9, 2024

wypb commented Oct 9, 2024

Yuhta commented Oct 9, 2024

wypb commented Oct 10, 2024

facebook-github-bot commented Oct 10, 2024

facebook-github-bot commented Oct 10, 2024

conbench-facebook bot commented Oct 10, 2024

Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and writer options #10895

Add sessionTimezone and adjustTimestampToTimezone to DWRF reader and writer options #10895

Conversation

wypb commented Aug 30, 2024 • edited by Yuhta Loading

netlify bot commented Aug 30, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

wypb commented Sep 6, 2024

mbasmanova commented Sep 9, 2024

facebook-github-bot commented Sep 9, 2024

wypb commented Sep 9, 2024 • edited Loading

facebook-github-bot commented Sep 9, 2024

Yuhta commented Sep 9, 2024

wypb commented Sep 9, 2024

Choose a reason for hiding this comment

wypb Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Sep 24, 2024

facebook-github-bot commented Sep 24, 2024

Yuhta Sep 25, 2024 • edited Loading

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

facebook-github-bot commented Oct 2, 2024

facebook-github-bot commented Oct 8, 2024

xiaoxmeng left a 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

wypb commented Oct 9, 2024

Yuhta commented Oct 9, 2024

wypb commented Oct 10, 2024

facebook-github-bot commented Oct 10, 2024

facebook-github-bot commented Oct 10, 2024

conbench-facebook bot commented Oct 10, 2024

wypb commented Aug 30, 2024 •

edited by Yuhta

Loading

netlify bot commented Aug 30, 2024 •

edited

Loading

wypb commented Sep 9, 2024 •

edited

Loading

wypb Sep 24, 2024 •

edited

Loading

Yuhta Sep 25, 2024 •

edited

Loading