Support different timestamp units in arrow bridge #7625

rui-mo · 2023-11-17T06:10:42Z

Arrow bridge supports different timestamp units, including second, milli, micro
and nano. This PR adds TimestampUnit in ArrowOptions to support these
units in the process of exportToArrow. For importFromArrow, the unit extracted
from arrow schema is followed. By default, the conversion unit is nano, and in
Gluten, micro is configured to align with Spark.
#4680 (comment)
Arrow Reference: https://github.com/apache/arrow/blob/main/cpp/src/arrow/c/bridge.cc#L402-L421.

netlify · 2023-11-17T06:10:47Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`bb74060`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65f94c2f839d2b0008f2aa83

rui-mo

One configuration on timestamp unit is added in connector config.

rui-mo · 2023-11-17T06:13:12Z

velox/dwio/parquet/writer/Writer.cpp

@@ -226,8 +226,8 @@ dwio::common::StripeProgress getStripeProgress(
 void Writer::write(const VectorPtr& data) {
  ArrowArray array;
  ArrowSchema schema;
-  exportToArrow(data, array, generalPool_.get());
-  exportToArrow(data, schema);
+  exportToArrow(data, options_, array, generalPool_.get());


The options in Writer is populated from connector config.

ViggoC · 2023-11-17T06:45:28Z

velox/core/QueryConfig.h

@@ -338,6 +338,10 @@ class QueryConfig {
  static constexpr const char* kEnableExpressionEvaluationCache =
      "enable_expression_evaluation_cache";

+  // Timestamp unit used during Velox-Arrow conversion.
+  static constexpr const char* kArrowBridgeTimestampUnit =


it's a query level config? But what if we have two timestamp fields with different precision?

Yes, different units among fields are not supported.

This is already an improvement over the previous situation. Are we considering going a step further to support configuration at the field level? However, that would entail significant changes, as it would require introducing a definition of timestamp precision in Velox's type system, along with the corresponding type inference.

Are we considering going a step further to support configuration at the field level?

We are testing with Spark, and at least so far no corresponding usage scenarios have been found. Spark provides configurable unit for writing and reading timstamps in Parquet, but it is context level and does not vary among fields. I'm wondering which system you are testing, and how common this issue is. cc @Yuhta

I'm from Bytedance. We are trying to integrate Velox in Elasticsearch. Elasticsearch support both date(ms) and date_nano(ns). We have not yet encountered a scenario where there are two timestamps of different precision in a query, though theoretically it exists.

I have an idea, we can pass through the expected arrow shcema to exportToArrow function, and exporting timestamp according to the expected unit of the timestamp field. @rui-mo what do you think about

But where can we get the expected arrow schema? The option controls the timestamp unit when exporting to arrow schema, so it is the same across columns.

As velox do not take responsibility for building query plan, the framework invoking velox must know what the output schema of velox task is. In this way, we can specify different timestamp unit across columns.

the framework invoking velox must know what the output schema of velox task is

I see your point. I assume the gap is Timestamp type in Velox does not hold a precision or unit field, so the caller cannot specify that information in the output schema. To achieve a differentiated timestamp unit when exporting to arrow, Velox vector or type needs to hold relating information I think.

One method I can think of is to pass through a ArrowSchema, like:

void exportToArrow( const VectorPtr& vec, ArrowSchema& arrowSchema, const ArrowOptions& options, const ArrowSchema* expectedArrowSchema)

ViggoC · 2023-11-28T09:57:08Z

velox/core/QueryConfig.h

@@ -549,6 +553,13 @@ class QueryConfig {
    return get<uint8_t>(kSpillStartPartitionBit, kDefaultStartBit);
  }

+  /// Returns the timestamp unit used in Velox-Arrow conversion.
+  /// 0: second, 1: milli, 2: micro, 3: nano.


how about define 0: second, 3: milli, 6: micro, 9: nano, means the number of digits of fractional seconds

Both should work. Maybe continous configuarable values are more common.

I'm not sure which system you're referring to with "continuous configurable values" for timestamp precision. AFAIK, in the timestamp syntax of MySQL, PostgreSQL, and Flink, the precision is indicated by the number of digits of fractional seconds.

I'm trying to say if we use discrete numbers, it may be confusing to user what happens if setting a value between. But I do not have strong opinion here. Both should work.

Understood. Theoretically, Velox could accommodate any precision between 0 and 9, but since Arrow only has three common precisions, such a configuration makes sense if it's solely for Arrow. However, IMHO, if we were to interface with another system that only supports millisecond and nanosecond, or supports any arbitrary precision like MySQL, this method of configuration could lead to confusion.

Thanks, makes sense. Will update according to your suggestion.

Updated. Thanks.

velox/vector/arrow/Bridge.cpp

Summary: This PR fixes the failure of `createTimestampVector` when nulls buffer is nullptr. #7625 (comment) Pull Request resolved: #8955 Reviewed By: xiaoxmeng Differential Revision: D54590232 Pulled By: Yuhta fbshipit-source-id: a41846bbad9f51f4569623b013044c3c1feac24f

Yuhta · 2024-03-12T20:10:58Z

velox/vector/arrow/Bridge.h

@@ -25,9 +25,17 @@
 struct ArrowArray;
 struct ArrowSchema;

+enum class TimestampUnit {


enum class TimestampUnit : uint8_t to match the one in writer options

Thanks for the comment. Fixed.

facebook-github-bot · 2024-03-13T14:21:48Z

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

Yuhta · 2024-03-14T15:46:48Z

@rui-mo Can you add the new config document in configs.rst

majetideepak

@rui-mo I was traveling early this week and was off the grid. Sorry for the delay.
The change looks good. Just a minor comment.

majetideepak · 2024-03-14T16:11:22Z

velox/connectors/hive/HiveConfig.h

@@ -247,6 +251,10 @@ class HiveConfig {

  bool s3UseProxyFromEnv() const;

+  /// Returns the timestamp unit used when exporting to Arrow.
+  /// 0: second, 3: milli, 6: micro, 9: nano.
+  uint8_t arrowBridgeTimestampUnit(const Config* session) const;


Does Arrow perform the error handling for values outside these?
Should Velox be performing a check as well?

Yes, Arrow throws exception for unexpected unit during conversion (link). Added a check here, thanks.

rui-mo · 2024-03-15T03:26:56Z

@Yuhta @majetideepak Appreciate your help on this PR. Your comments were addressed.

majetideepak

Thanks, @rui-mo

facebook-github-bot · 2024-03-15T14:46:05Z

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

Yuhta · 2024-03-15T16:05:46Z

velox/connectors/hive/HiveConfig.h

@@ -178,6 +178,12 @@ class HiveConfig {
  static constexpr const char* kS3UseProxyFromEnv =
      "hive.s3.use-proxy-from-env";

+  /// Timestamp unit used during Velox-Arrow conversion.
+  static constexpr const char* kArrowBridgeTimestampUnit =
+      "arrow-bridge-timestamp-unit";


@rui-mo Can we prefix it with hive.parquet.writer. to be specific and consistent?

Updated as suggested. Thanks.

facebook-github-bot · 2024-03-18T16:01:34Z

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

mbasmanova · 2024-03-18T16:50:11Z

velox/docs/configs.rst

+     - hive.parquet.writer.timestamp_unit
+     - tinyint
+     - 9
+     - Timestamp unit used when writing timestamps into Parquet through Arrow bridge.


@rui-mo Would you clarify what are valid values? It may not be obvious.

Updated the documentation. Thanks.

mbasmanova

@rui-mo Thanks.

facebook-github-bot · 2024-03-19T16:04:59Z

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

facebook-github-bot · 2024-03-19T20:14:50Z

@Yuhta merged this pull request in a942ac8.

kgpai · 2024-03-19T23:58:06Z

@rui-mo This change breaks on ubuntu and gcc 11 : https://github.com/facebookincubator/velox/actions/runs/8350541386/job/22857176679?pr=8734
(It looks like a minor thing , most likely it wasnt flagged because we dont do a gcc 11 build here )

rui-mo · 2024-03-20T00:58:01Z

@kgpai I see. This commit cb0ce40 should fix the compilation error. Do I need to open a PR on main, or we can include this change in #8734? Thanks.

ViggoC · 2024-03-20T05:19:05Z

velox/vector/arrow/Bridge.cpp

+  if (!vec.mayHaveNulls() || vec.getNullCount() == 0) {
+    switch (unit) {
+      case TimestampUnit::kSecond:
+        rows.apply([&](vector_size_t i) { dst[i] = src[i].getSeconds(); });


@rui-mo we can not just set data to dst[i], i is not start from 0 if the offset in rows is not zero.

@ViggoC Thank you so much for catching this. Filed #9170 to fix this issue. Would you help take a review?

kgpai · 2024-03-20T17:09:26Z

@rui-mo I created a separate pr : #9163 and should land soon. Thanks !

Summary: This PR fixes the failure of `createTimestampVector` when nulls buffer is nullptr. facebookincubator#7625 (comment) Pull Request resolved: facebookincubator#8955 Reviewed By: xiaoxmeng Differential Revision: D54590232 Pulled By: Yuhta fbshipit-source-id: a41846bbad9f51f4569623b013044c3c1feac24f

…7625) Summary: Arrow bridge supports different timestamp units, including second, milli, micro and nano. This PR adds `TimestampUnit` in `ArrowOptions` to support these units in the process of exportToArrow. For importFromArrow, the unit extracted from arrow schema is followed. By default, the conversion unit is nano, and in Gluten, micro is configured to align with Spark. facebookincubator#4680 (comment) Arrow Reference: https://github.com/apache/arrow/blob/main/cpp/src/arrow/c/bridge.cc#L402-L421. Pull Request resolved: facebookincubator#7625 Reviewed By: mbasmanova Differential Revision: D54852534 Pulled By: Yuhta fbshipit-source-id: 0494102fedc73f7068424bc09e972e7deb297a6e

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 Nov 17, 2023

rui-mo mentioned this pull request Nov 17, 2023

Support for dictionary encoded INT96 timestamp in parquet files #4680

Closed

rui-mo commented Nov 17, 2023

View reviewed changes

rui-mo force-pushed the wip_bridge branch 2 times, most recently from 1a49c1c to 44b270a Compare November 17, 2023 06:40

ViggoC reviewed Nov 17, 2023

View reviewed changes

rui-mo force-pushed the wip_bridge branch from 44b270a to ecded34 Compare November 24, 2023 00:43

JkSelf mentioned this pull request Nov 27, 2023

[VL] enable 11-22 upstreaming velox rebase apache/incubator-gluten#3773

Merged

ViggoC reviewed Nov 28, 2023

View reviewed changes

rui-mo force-pushed the wip_bridge branch 3 times, most recently from e87eac5 to f5586b9 Compare December 1, 2023 05:20

rui-mo force-pushed the wip_bridge branch 2 times, most recently from 016f01c to 93f1ff7 Compare December 6, 2023 05:48

rui-mo force-pushed the wip_bridge branch 2 times, most recently from 631eca4 to 955cbd4 Compare January 3, 2024 03:08

rui-mo force-pushed the wip_bridge branch from 955cbd4 to 7eca4d5 Compare January 15, 2024 02:41

rui-mo force-pushed the wip_bridge branch from 7eca4d5 to 764fceb Compare January 26, 2024 02:41

rui-mo force-pushed the wip_bridge branch from 764fceb to 9e1ee2d Compare February 2, 2024 08:20

rui-mo force-pushed the wip_bridge branch 2 times, most recently from 4581ed9 to 63f5de1 Compare February 26, 2024 05:33

rui-mo referenced this pull request in oap-project/velox Feb 28, 2024

Support different timestamp units in arrow bridge (7625)

64adf58

ViggoC reviewed Feb 29, 2024

View reviewed changes

velox/vector/arrow/Bridge.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_bridge branch from 63f5de1 to c27fff3 Compare March 1, 2024 07:37

ViggoC reviewed Mar 1, 2024

View reviewed changes

velox/vector/arrow/Bridge.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_bridge branch from c27fff3 to e1454c7 Compare March 4, 2024 08:56

rui-mo mentioned this pull request Mar 5, 2024

Fix createTimestampVector without nulls buffer #8955

Closed

rui-mo force-pushed the wip_bridge branch from e1454c7 to 6a27173 Compare March 7, 2024 06:20

Yuhta reviewed Mar 12, 2024

View reviewed changes

rui-mo force-pushed the wip_bridge branch from 63baa90 to 15e787e Compare March 13, 2024 06:01

majetideepak approved these changes Mar 14, 2024

View reviewed changes

rui-mo force-pushed the wip_bridge branch 2 times, most recently from 04ffe5b to 303d386 Compare March 15, 2024 02:59

majetideepak approved these changes Mar 15, 2024

View reviewed changes

Yuhta reviewed Mar 15, 2024

View reviewed changes

rui-mo force-pushed the wip_bridge branch from 303d386 to 145608a Compare March 18, 2024 13:39

mbasmanova reviewed Mar 18, 2024

View reviewed changes

rui-mo force-pushed the wip_bridge branch 2 times, most recently from 3d6d5e9 to 65eca68 Compare March 19, 2024 00:59

Support timestamp units in arrow bridge

bb74060

rui-mo force-pushed the wip_bridge branch from 65eca68 to bb74060 Compare March 19, 2024 08:26

mbasmanova approved these changes Mar 19, 2024

View reviewed changes

facebook-github-bot closed this in a942ac8 Mar 19, 2024

facebook-github-bot added the Merged label Mar 19, 2024

ViggoC reviewed Mar 20, 2024

View reviewed changes

Support different timestamp units in arrow bridge #7625

Support different timestamp units in arrow bridge #7625

Conversation

rui-mo commented Nov 17, 2023 • edited Loading

netlify bot commented Nov 17, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

rui-mo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo Nov 29, 2023 • 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

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

Choose a reason for hiding this comment

ViggoC Nov 29, 2023 • 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

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 Mar 13, 2024

Yuhta commented Mar 14, 2024

majetideepak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo commented Mar 15, 2024

majetideepak left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 15, 2024

Yuhta Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 19, 2024

facebook-github-bot commented Mar 19, 2024

kgpai commented Mar 19, 2024

rui-mo commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgpai commented Mar 20, 2024

rui-mo commented Nov 17, 2023 •

edited

Loading

netlify bot commented Nov 17, 2023 •

edited

Loading

rui-mo left a comment •

edited

Loading

rui-mo Nov 29, 2023 •

edited

Loading

ViggoC Nov 29, 2023 •

edited

Loading

Yuhta Mar 15, 2024 •

edited

Loading