Support date-type partition column #7084

rui-mo · 2023-10-17T02:24:38Z

When the partition column is of date type, below error occurs when trying to
convert date string as int. This PR fixes this exception by using
castFromDateString with isIso8601 set as true. With this fix, the date
partition value formatted as string should be in the [+-](YYYY-MM-DD) format.
The callers like Gluten need to generate compatible format of strings to be
partition values.

Exceptions.h:69] Line: ../../velox/exec/Driver.cpp:545, Function:runInternal, Expression:  Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-whitespace character found after end of conversion: "-10-27", Source: RUNTIME, ErrorCode: INVALID_STATE
unknown file: Failure
C++ exception with description "Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-whitespace character found after end of conversion: "-10-27"

netlify · 2023-10-17T02:24:44Z

✅ Deploy Preview for meta-velox canceled.

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

rui-mo · 2023-10-31T09:04:17Z

@mbasmanova Could you spare some time to review this PR? Thanks.

mbasmanova · 2023-10-31T11:27:11Z

velox/connectors/Connector.h

@@ -307,6 +307,14 @@ class ConnectorQueryCtx {
    return planNodeId_;
  }

+  void setCastStringToDateIsIso8601(bool flag) {


This doesn't look right. Shouldn't it be part of the config?

The transfer chain of this value is as queryConfig -> createDataSource -> ReaderOptions -> HiveDataSource -> splitReader_->prepareSplit -> adaptColumns -> setPartitionValue.
I want to get it from queryConfig and set it to connectorQueryCtx->config() before createDataSource, but found Config does not allow adding properties at runtime.
https://github.com/facebookincubator/velox/pull/7084/files#diff-b04232f155cc15faca93b69cf34dd80e29b534a4e54dd774bf8bdbedbfc66d6bR127-R128

This doesn't feel right. HiveConnector should define its own configuration properties and use these. We should not assume that castStringToDateIsIso8601_ config applies to all connectors.

I'm still trying to understand why do you need this. I don't see tests for both cases when castStringToDateIsIso8601_ is true and when it is false. Am I missing something?

Should we add a configuration property to velox/connectors/hive/HiveConfig.h?

This config was introduced by 8d6c296, so I assume it can be reused to control the cast behavior in setPartitionValue. Tests for castStringToDateIsIso8601_ being true and false are already covered by that commit.

@rui-mo At a high level, engine configs are separate from connector config. We should not leak engine-specific logic into connectors. I wonder why do we pass partition values in HiveConnectorSplit as string. Should we pass these as values of proper types (e.g. ConstantVectors)? Will this remove the need to converting from strings to proper types?

I also see that applyPartitionFilter may need to be updated to handle DATE type.

Another option might be for the connector to access cast logic in the engine via ExpressionEvaluator, i.e. construct and evaluate a CAST expression.

Masha, thank you for the suggestion. For option 1, Velox now uses a map as below to store partition keys in HiveConnectorSplit.h.

const std::unordered_map<std::string, std::optionalstd::string> partitionKeys

Seems const std::unordered_map<std::string, core::ConstantTypedExpr> partitionKeys can help remove the cast process. But I'm not sure if there's any special consideration in Velox to use string type. In Gluten, we need to convert literals to string type to create partition keys, and in Velox strings are converted to variants. If ConstantTypedExpr is used, the intermediate string could be removed.

const std::unordered_map<std::string, core::ConstantTypedExpr> partitionKeys

This could work. Would you like to prototype this change? CC: @Yuhta

Also, wondering if any changes are needed to support writing to partitioned tables with DATE partition keys.

velox/connectors/hive/SplitReader.cpp

mbasmanova

Some questions.

@Yuhta Jimmy, would you help review this PR?

CC: @kevinwilfong

Yuhta · 2023-11-02T20:10:45Z

Is partition just a directory name in Hive? If yes it is always a string right? Or it can be stored as some typed metadata?

If the original source of the partition value is a string, I think casting it in connector makes sense. Otherwise we can try passing in the typed value directly.

rui-mo · 2023-11-03T01:10:07Z

Is partition just a directory name in Hive? If yes it is always a string right? Or it can be stored as some typed metadata?

@mbasmanova @Yuhta Partition keys are needed when creating HiveConnectorSplit. In Gluten, typed constant can be got from Spark so we don't need string, but I'm not sure about other engines.

mbasmanova · 2023-11-03T13:09:32Z

Partition keys can be any type and queries may need to return these as columns. Hence, all engines should be able to specify typed values for partition keys.

Yuhta · 2023-11-03T14:59:40Z

@mbasmanova I understand that we need to return partition value as a typed column in query result. But how is it stored in Hive? Is it type + string, or type + type specific blob? In the first case we can do the conversion in Velox, in the second case Velox should get the typed constant vector I think.

rui-mo · 2024-02-01T09:23:33Z

Hi @mbasmanova @Yuhta, it has been a long time since last discussion. Apology for the delay and I have made some updates. Would you like to take a review again?

I checked how partitioned table was stored in Hive, and found sub-directory was used. I assume that's why partitions values are passed as string to HiveConnectorSplit, and the conversion from string to typed variant is needed.

Hive internally splits the records based on the partition key and stores each partition data into a sub-directory of tables directory on HDFS. The name of the directory would be partition key and it's value.

Previously, to support the conversion from string to date, I used castFromDateString method, but got confused on how to distinguish isIso8601 flag for Presto and Spark. At present, this issue can be solved by constructing and evaluating a cast expression, as its semantics will follow the registered CastExpr.

mbasmanova · 2024-02-01T11:31:58Z

velox/connectors/hive/SplitReader.cpp

+        expressionEvaluator->compile(callExpr);
+    VectorPtr result;
+    const SelectivityVector rows(1);
+    expressionEvaluator->evaluate(exprSet.get(), rows, *input, result);


Using expression evaluation here feels a bit heavy weight. Why can't we use a lower-level utility? Do we not have access to query config?

Thanks for your review. These was a deprecated query config isIso8601, but could not be leaked to connector as previous discussion #7084 (comment) pointed out. Do you think it makes sense to add a configuration in HiveConfig? BTW, the semantics of cast string as timestamp are also different between Presto and Spark. Do we intend to use configurations to solve them? (Currently timestamp is kept as is to use Converter because of a workaround to handle timezone.)

Why can't we use a lower-level utility?

Hi @mbasmanova, I took a further look here. The format of partitionKeys can be controlled by Gluten, so if we ensure that compatible strings are generated as partition values, maybe we don't need to bring more complexity here. For date format YYYY-MM-DD, castFromDateString with true for isIso8601 should be enough. Do you think it makes sense?

mbasmanova · 2024-02-01T17:55:32Z

velox/exec/tests/TableScanTest.cpp

+  auto filePath = TempFilePath::create();
+  writeToFile(filePath->path, vectors);
+  createDuckDbTable(vectors);
+  testPartitionedTable(filePath->path, DATE(), "2023-10-27");


These was a deprecated query config isIso8601, but could not be leaked to connector as previous discussion #7084 (comment) pointed out

I'm remembering now. What other date formats are supported? Can we add test cases to make sure all supported formats work and different format are supported based on what version of CAST is registered?

I notice we can control in Gluten to generate well-formatted strings as partition values. Maybe we can keep using light-weight conversion here.

Maybe we can keep using light-weight conversion here.

That would be great.

Updated. Could you help review again? Thanks.

mbasmanova

@rui-mo Looks great % one nit.

Would you update PR description to provide fuller context about the issue and the solution, including the logic you are planning to add to Gluten?

mbasmanova · 2024-02-02T10:51:10Z

velox/connectors/hive/SplitReader.cpp

  if (value.has_value()) {
    if constexpr (ToKind == TypeKind::VARCHAR) {
      return velox::variant(value.value());
    }
    if constexpr (ToKind == TypeKind::VARBINARY) {
      return velox::variant::binary((value.value()));
    }
+    if (toType->isDate()) {


Would you add documentation to HiveConnectorSplit::partitionKeys to clarify what values / types are supported and how they are converted from the std::string?

Thanks for the suggestion. Updated PR description and documentation.

facebook-github-bot · 2024-02-02T10:52:53Z

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

mbasmanova · 2024-02-02T13:39:59Z

velox/connectors/hive/HiveConnectorSplit.h

@@ -27,6 +27,10 @@ struct HiveConnectorSplit : public connector::ConnectorSplit {
  dwio::common::FileFormat fileFormat;
  const uint64_t start;
  const uint64_t length;
+
+  /// A map stores partition keys and values in string. Typed partition values


perhaps,

Mapping from partition keys to values. Values are specified as strings formatted the same way as CAST(x as VARCHAR). Null values are specified as std::nullopt. Date values must be formatted using ISO 8601 as YYYY-MM-DD.

All scalar types and date type are supported.

Is this so? Do we support real, double and timestamp? What's the format expected for these?

Yes, real, double and timestamp are supported in Converter (real and double, timestamp). The expected format should be the same as cast expects real and double, timestamp

I was wondering whether Spark support real, double, timestamp as partition keys.

They are supported in Spark, but boolean type is not. There is one test on partition column types in Spark link.

Got it. Would it be possible to add tests for all supported types in a follow-up ?

Yes, I will follow up in a separate PR. Through first glance, misssing tests are on decimal and timestamp.

@mbasmanova I double-checked and found only decimal was missing. Opened #8670 to add.

facebook-github-bot · 2024-02-02T13:59:41Z

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

facebook-github-bot · 2024-02-02T14:23:39Z

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

facebook-github-bot · 2024-02-02T17:30:15Z

@mbasmanova merged this pull request in 78aa782.

conbench-facebook · 2024-02-02T17:52:57Z

Conbench analyzed the 1 benchmark run on commit 78aa7825.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: When the partition column is of date type, below error occurs when trying to convert date string as int. This PR fixes this exception by using `castFromDateString` with `isIso8601` set as true. With this fix, the date partition value formatted as string should be in the `[+-](YYYY-MM-DD)` format. The callers like Gluten need to generate compatible format of strings to be partition values. ``` Exceptions.h:69] Line: ../../velox/exec/Driver.cpp:545, Function:runInternal, Expression: Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-whitespace character found after end of conversion: "-10-27", Source: RUNTIME, ErrorCode: INVALID_STATE unknown file: Failure C++ exception with description "Exception: VeloxRuntimeError Error Source: RUNTIME Error Code: INVALID_STATE Reason: Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-whitespace character found after end of conversion: "-10-27" ``` Pull Request resolved: facebookincubator#7084 Reviewed By: pedroerp Differential Revision: D53348851 Pulled By: mbasmanova fbshipit-source-id: 38f9ae484a9976ddf08f1805fa7d10baa16c2fc5

Summary: When the partition column is of date type and a filter is applied on it, below error occurs when trying to convert date string as int. This PR fixes this exception by utilizing castFromDateString with standard cast behavior. ``` C++ exception with description "Exception: VeloxRuntimeError Error Source: RUNTIME Error Code: INVALID_STATE Reason: Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-whitespace character found after end of conversion: "-10-27" ``` #7084 Pull Request resolved: #9937 Reviewed By: pedroerp Differential Revision: D57913463 Pulled By: Yuhta fbshipit-source-id: 7956de4ae122b080ecf80a96cfcf546f8aae7efe

Summary: When the partition column is of date type and a filter is applied on it, below error occurs when trying to convert date string as int. This PR fixes this exception by utilizing castFromDateString with standard cast behavior. ``` C++ exception with description "Exception: VeloxRuntimeError Error Source: RUNTIME Error Code: INVALID_STATE Reason: Operator::getOutput failed for [operator: TableScan, plan node ID: 0]: Non-whitespace character found after end of conversion: "-10-27" ``` facebookincubator#7084 Pull Request resolved: facebookincubator#9937 Reviewed By: pedroerp Differential Revision: D57913463 Pulled By: Yuhta fbshipit-source-id: 7956de4ae122b080ecf80a96cfcf546f8aae7efe

rui-mo marked this pull request as draft October 17, 2023 02:24

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

rui-mo force-pushed the wip_date branch from c8cf692 to 8800e86 Compare October 27, 2023 06:26

rui-mo marked this pull request as ready for review October 27, 2023 06:27

rui-mo force-pushed the wip_date branch from 8800e86 to 88fee52 Compare October 31, 2023 09:03

mbasmanova requested a review from Yuhta October 31, 2023 11:01

mbasmanova reviewed Oct 31, 2023

View reviewed changes

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

mbasmanova reviewed Oct 31, 2023

View reviewed changes

rui-mo force-pushed the wip_date branch from 88fee52 to a19696c Compare November 1, 2023 03:40

rui-mo force-pushed the wip_date branch from a19696c to 86063ea Compare January 3, 2024 01:52

rui-mo force-pushed the wip_date branch from 86063ea to 0dd42e6 Compare January 15, 2024 02:46

rui-mo marked this pull request as draft January 30, 2024 01:16

rui-mo force-pushed the wip_date branch 3 times, most recently from b3ed5ed to 37bfc06 Compare February 1, 2024 07:53

rui-mo changed the title ~~Support date as partition value type~~ Support date-type partition column Feb 1, 2024

rui-mo force-pushed the wip_date branch from 37bfc06 to 777ec45 Compare February 1, 2024 08:53

rui-mo marked this pull request as ready for review February 1, 2024 08:55

mbasmanova reviewed Feb 1, 2024

View reviewed changes

rui-mo force-pushed the wip_date branch 2 times, most recently from 2fad518 to 7e9aabe Compare February 2, 2024 10:40

mbasmanova approved these changes Feb 2, 2024

View reviewed changes

rui-mo force-pushed the wip_date branch from 7e9aabe to 6cf148f Compare February 2, 2024 13:31

mbasmanova reviewed Feb 2, 2024

View reviewed changes

date-type partition column

a74b5b5

rui-mo force-pushed the wip_date branch from 6cf148f to a74b5b5 Compare February 2, 2024 13:56

facebook-github-bot closed this in 78aa782 Feb 2, 2024

facebook-github-bot added the Merged label Feb 2, 2024

rui-mo mentioned this pull request May 27, 2024

Support date-type partition filter #9937

Closed

Support date-type partition column #7084

Support date-type partition column #7084

Conversation

rui-mo commented Oct 17, 2023 • edited Loading

netlify bot commented Oct 17, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

rui-mo commented Oct 31, 2023

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

mbasmanova left a comment

Choose a reason for hiding this comment

Yuhta commented Nov 2, 2023 • edited Loading

rui-mo commented Nov 3, 2023

mbasmanova commented Nov 3, 2023

Yuhta commented Nov 3, 2023

rui-mo commented Feb 1, 2024

Choose a reason for hiding this comment

rui-mo Feb 1, 2024 • 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 Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 2, 2024

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 Feb 2, 2024

facebook-github-bot commented Feb 2, 2024

facebook-github-bot commented Feb 2, 2024

conbench-facebook bot commented Feb 2, 2024

rui-mo commented Oct 17, 2023 •

edited

Loading

netlify bot commented Oct 17, 2023 •

edited

Loading

Yuhta commented Nov 2, 2023 •

edited

Loading

rui-mo Feb 1, 2024 •

edited

Loading

rui-mo Feb 2, 2024 •

edited

Loading