Support all Spark patterns in cast(varchar as date) #5844

marin-ma · 2023-07-26T11:39:25Z

Below patterns are considered valid to cast from string to date in spark sql functions:

Year only: "YYYY"
Year and month only: "YYYY-MM"
Any characters after trailing spaces: "YYYY-MM-DD ", "YYYY-MM-DD 123", "YYYY-MM-DD (BC)"
Any characters after trailing character 'T': "YYYY-MM-DDT"

Below patterns are invalid:

Year is too large (exceed INT32_MAX): 20150318
Other separators "YYYY/MM/DD"

Reference:
Spark cast from string to date:
https://github.com/apache/spark/blob/3e5203c64c06cc8a8560dfa0fb6f52e74589b583/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala#L286-L298

Unit test:
CastExprTest::fromStringToDate is derived from https://github.com/apache/spark/blob/3a9185964a0de3c720a6b77d38a446258b73468e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala#L103-L126
CastExprTest::fromStringToDateInvalid is derived from
https://github.com/apache/spark/blob/3a9185964a0de3c720a6b77d38a446258b73468e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOffSuite.scala#L67-L73

netlify · 2023-07-26T11:39:37Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`0fbc6a2`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64fc1715b45c7900082146df
😎 Deploy Preview	https://deploy-preview-5844--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2023-07-26T11:39:50Z

👷 Deploy Preview for meta-velox processing.

Name	Link
🔨 Latest commit	`c461566`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/64c105eec2fa6300087f904f

marin-ma · 2023-08-01T14:00:25Z

@mbasmanova PTAL. Thanks!

mbasmanova

@marin-ma Does Presto support these patterns as well? Would you update documentation for CAST to add these new patterns?

#5847

CC: @kagamiori

marin-ma · 2023-08-01T14:37:41Z

@marin-ma Does Presto support these patterns as well? Would you update documentation for CAST to add these new patterns?

#5847

CC: @kagamiori

I'm not sure about prestosql. Neither do I have an test environment to check. @kagamiori Could you help to check on prestosql?

marin-ma · 2023-08-01T15:28:15Z

I just got a test environment for prestosql. Only SELECT cast('YYYY-MM-DD' as date) pass and all other patterns fails. Looks like prestosql only support one pattern 'YYYY-MM-DD' @kagamiori

mbasmanova · 2023-08-01T15:31:51Z

@marin-ma Thank you for checking. Sounds like Presto and Spark behavior for cast(varchar as date) is different. Since, CastExpr is shared between the two, we would need to introduce a configuration property to control the behavior. Search for queryConfig.isCastToIntByTruncate() to see some examples.

kagamiori · 2023-08-01T18:57:14Z

I just got a test environment for prestosql. Only SELECT cast('YYYY-MM-DD' as date) pass and all other patterns fails. Looks like prestosql only support one pattern 'YYYY-MM-DD' @kagamiori

Double checked on my side too. These (except YYYY-MM-DD) are not supported in Presto.

marin-ma · 2023-08-02T14:45:02Z

@marin-ma Thank you for checking. Sounds like Presto and Spark behavior for cast(varchar as date) is different. Since, CastExpr is shared between the two, we would need to introduce a configuration property to control the behavior. Search for queryConfig.isCastToIntByTruncate() to see some examples.

@mbasmanova Updated & PTAL. Thanks!

mbasmanova · 2023-08-02T14:48:28Z

velox/type/TimestampConversion.cpp

@@ -105,6 +105,8 @@ constexpr int32_t kCumulativeYearDays[] = {

 namespace {

+enum class ParseMode { STRICT, NON_STRICT, STANDARD_CAST, NON_STANDARD_CAST };


Please, review the coding guidelines and update accordingly.

mbasmanova · 2023-08-02T14:48:52Z

velox/type/TimestampConversion.h

+/// `[+-]YYYY*-[M]M-[D]DT*`
+///
+/// Throws VeloxUserError if the format or date is invalid.
+int32_t castFromDateString(const char* buf, size_t len, bool isNonStandardCast);


It would be nice to extract this change into a separate PR and add tests.

Summary: This is a splitting PR from #5844 Since `fromDateString` cannot properly handle different cast behaviors and returns int64_t that may cause overflow of Date type, this PR add `castFromDateString` as a helper function, which can be used for handle different cast from string to date behavior from sparksql and prestosql. Below patterns are considered valid to cast from string to date in spark sql functions: Year only: "YYYY" Year and month only: "YYYY-MM" Digits after trailing spaces: "YYYY-MM-DD 123" Trailing character 'T': "YYYY-MM-DDT" Digits after 'T': "YYYY-MM-DDT123123" "[+-]" before any valid pattern. Below patterns are valid in presto sql: "[+-]YYYY-MM-DD" Pull Request resolved: #5994 Reviewed By: kgpai Differential Revision: D48234502 Pulled By: bikramSingh91 fbshipit-source-id: 1e773692cad438bc5d3b948b0ab33e5f39f89823

marin-ma · 2023-08-15T07:16:00Z

@bikramSingh91 Could you help to review?

marin-ma · 2023-08-17T03:10:43Z

@bikramSingh91 Could you help to review this PR? This one is a follow-up of #5994 Thanks!

…#5994) Summary: This is a splitting PR from facebookincubator#5844 Since `fromDateString` cannot properly handle different cast behaviors and returns int64_t that may cause overflow of Date type, this PR add `castFromDateString` as a helper function, which can be used for handle different cast from string to date behavior from sparksql and prestosql. Below patterns are considered valid to cast from string to date in spark sql functions: Year only: "YYYY" Year and month only: "YYYY-MM" Digits after trailing spaces: "YYYY-MM-DD 123" Trailing character 'T': "YYYY-MM-DDT" Digits after 'T': "YYYY-MM-DDT123123" "[+-]" before any valid pattern. Below patterns are valid in presto sql: "[+-]YYYY-MM-DD" Pull Request resolved: facebookincubator#5994 Reviewed By: kgpai Differential Revision: D48234502 Pulled By: bikramSingh91 fbshipit-source-id: 1e773692cad438bc5d3b948b0ab33e5f39f89823

marin-ma · 2023-08-22T01:58:52Z

@mbasmanova Could you help to review again?

bikramSingh91 · 2023-08-22T20:45:31Z

velox/expression/tests/CastExprTest.cpp

@@ -651,6 +658,68 @@ TEST_F(CastExprTest, invalidDate) {
      "date", {"2012-Oct-23"}, {0}, true, false, VARCHAR(), DATE());
 }

+TEST_F(CastExprTest, stringToDate) {


nit: can you move these tests to TEST_F(CastExprTest, date)

bikramSingh91 · 2023-08-22T20:45:53Z

velox/expression/tests/CastExprTest.cpp

+      DATE());
+}
+
+TEST_F(CastExprTest, stringToDateInvalid) {


nit: can you move these tests to TEST_F(CastExprTest, invalidDate)

bikramSingh91 · 2023-08-22T20:48:53Z

velox/docs/configs.rst

+     - bool
+     - false
+     - This flags allows the cast from string to date accept patterns other than the "[+-](YYYY-MM-DD)" format.
+       Valid patterns include (YYYY, YYYY-MM, YYYY-MM-DD), and any patterns prefixed with [+-]".


should we also mention other cases like
Digits after trailing spaces: "YYYY-MM-DD 123"
Trailing character 'T': "YYYY-MM-DDT"
Digits after 'T': "YYYY-MM-DDT123123"

bikramSingh91 · 2023-08-22T20:53:05Z

velox/expression/tests/CastExprTest.cpp

+  for (bool conf : {true, false}) {
+    setCastStringToDateNonStandard(conf);
+    testCast<std::string, int32_t>(
+        "date",


wondering if we should include a case for trailing (BC) for the case where it is valid

Added. It's invalid in presto but valid in spark.

@bikramSingh91 My previous understanding of trailing (BC) was wrong. The trailing (BC) starts with an additional space. So cast("2015-03-18(BC)" as date returns NULL in spark but cast("2015-03-18 (BC)" as date returns 2015-03-18

bikramSingh91 · 2023-08-22T21:35:54Z

velox/core/QueryConfig.h

@@ -80,6 +80,11 @@ class QueryConfig {
  // truncating the decimal part instead of rounding.
  static constexpr const char* kCastToIntByTruncate = "cast_to_int_by_truncate";

+  // This flags allows the cast from string to date accept patterns other than
+  // the "(+-)[YYYY-MM-DD]" format.
+  static constexpr const char* kCastStringToDateNonStandard =


nit: can you also mention this newly added query config in the PR description that enables this behavior.

mbasmanova · 2023-08-22T22:00:34Z

velox/docs/configs.rst

@@ -109,6 +109,11 @@ Expression Evaluation Configuration
     - bool
     - false
     - This flags forces the cast from float/double to integer to be performed by truncating the decimal part instead of rounding.
+   * - cast_string_to_date_non_standard


Let's also document configs that alter cast behavior in cast documentation: https://facebookincubator.github.io/velox/functions/presto/conversion.html

@mbasmanova "non_standard" here means non ISO8601 standard. Or change it to cast_string_to_date_iso8601 and make the default value true?

marin-ma · 2023-08-24T04:07:21Z

@mbasmanova @bikramSingh91 Could you help to review again? Thanks!

bikramSingh91 · 2023-08-31T20:38:48Z

@marin-ma Can you please take a look at the failure in castFromDateStringInvalid that failing some of the circleCI precommits?

facebook-github-bot · 2023-08-31T20:39:50Z

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

marin-ma · 2023-09-01T01:59:15Z

@marin-ma Can you please take a look at the failure in castFromDateStringInvalid that failing some of the circleCI precommits?

@bikramSingh91 PTAL. Thanks!

facebook-github-bot · 2023-09-01T21:56:49Z

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

marin-ma · 2023-09-05T01:14:44Z

@bikramSingh91 Could you help to check the error message of Linter? I can't see the errors.

bikramSingh91 · 2023-09-05T16:54:38Z

velox/type/tests/TimestampConversionTest.cpp

@@ -15,6 +15,7 @@
 */

 #include "velox/type/TimestampConversion.h"
+#include <common/base/tests/GTestUtils.h>


nit: can you please update this to use
#include "velox/common/base/tests/GTestUtils.h"
and add it under L21 instead

jinchengchenghh · 2023-08-28T02:37:25Z

velox/type/TimestampConversion.cpp

@@ -270,6 +270,15 @@ bool tryParseDateString(

  // In standard-cast mode, no more trailing characters.


This comment is outdated In standard-cast mode, no more trailing characters.

jinchengchenghh · 2023-09-06T01:07:23Z

velox/type/tests/TimestampConversionTest.cpp

@@ -15,6 +15,7 @@
 */

 #include "velox/type/TimestampConversion.h"
+#include <common/base/tests/GTestUtils.h>
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>


Include #include "velox/common/base/tests/GTestUtils.h" we can remove #include <gtest/gtest.h>

facebook-github-bot · 2023-09-06T17:01:10Z

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

bikramSingh91 · 2023-09-06T23:06:03Z

velox/type/TimestampConversion.cpp

+    // Skip trailing spaces.
+    while (pos < len && characterIsSpace(buf[pos])) {
+      pos++;
+    }
+    // Check position. if end was not reached, non-space chars remaining.
+    if (pos < len) {
+      return false;
+    }
+


@marin-ma Thanks for making all the changes till now. I have a last request, can you please move this part of the change out in a separate PR? This unfortunately, is a change in behavior of existing cast and can result in issues for existing workloads that dont expect this behavior. It would be good to separate it in case we need to roll back this behavior change while still keeping all the spark specific functionality you added here.

facebook-github-bot · 2023-09-07T18:43:12Z

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

bikramSingh91 · 2023-09-07T18:44:52Z

Thank you for moving out the changes to standard mode. I'll import these updates and will proceed with merging.

marin-ma · 2023-09-11T00:56:14Z

@bikramSingh91 Removed the changes in the latest commit. Could you help review again? Thanks!

marin-ma · 2023-09-13T01:16:39Z

@bikramSingh91 Could you help to review again? Thanks!

facebook-github-bot · 2023-09-14T17:46:36Z

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

bikramSingh91 · 2023-09-14T17:47:25Z

@marin-ma I was on PTO for that last week, will take this up today with high priority

facebook-github-bot · 2023-09-15T00:18:24Z

@bikramSingh91 merged this pull request in 8d6c296.

…r#5844) Summary: Below patterns are considered valid to cast from string to date in spark sql functions: 1. Year only: "YYYY" 2. Year and month only: "YYYY-MM" 3. Any characters after trailing spaces: "YYYY-MM-DD ", "YYYY-MM-DD 123", "YYYY-MM-DD (BC)" 4. Any characters after trailing character 'T': "YYYY-MM-DDT" Below patterns are invalid: 1. Year is too large (exceed INT32_MAX): 20150318 2. Other separators "YYYY/MM/DD" Reference: Spark cast from string to date: https://github.com/apache/spark/blob/3e5203c64c06cc8a8560dfa0fb6f52e74589b583/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala#L286-L298 Unit test: CastExprTest::fromStringToDate is derived from https://github.com/apache/spark/blob/3a9185964a0de3c720a6b77d38a446258b73468e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala#L103-L126 CastExprTest::fromStringToDateInvalid is derived from https://github.com/apache/spark/blob/3a9185964a0de3c720a6b77d38a446258b73468e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOffSuite.scala#L67-L73 Pull Request resolved: facebookincubator#5844 Reviewed By: kevinwilfong Differential Revision: D48881690 Pulled By: bikramSingh91 fbshipit-source-id: 4236669585cf76762da1bc96d7976014aebab5d3

…#5994) Summary: This is a splitting PR from facebookincubator#5844 Since `fromDateString` cannot properly handle different cast behaviors and returns int64_t that may cause overflow of Date type, this PR add `castFromDateString` as a helper function, which can be used for handle different cast from string to date behavior from sparksql and prestosql. Below patterns are considered valid to cast from string to date in spark sql functions: Year only: "YYYY" Year and month only: "YYYY-MM" Digits after trailing spaces: "YYYY-MM-DD 123" Trailing character 'T': "YYYY-MM-DDT" Digits after 'T': "YYYY-MM-DDT123123" "[+-]" before any valid pattern. Below patterns are valid in presto sql: "[+-]YYYY-MM-DD" Pull Request resolved: facebookincubator#5994 Reviewed By: kgpai Differential Revision: D48234502 Pulled By: bikramSingh91 fbshipit-source-id: 1e773692cad438bc5d3b948b0ab33e5f39f89823

…r#5844) Summary: Below patterns are considered valid to cast from string to date in spark sql functions: 1. Year only: "YYYY" 2. Year and month only: "YYYY-MM" 3. Any characters after trailing spaces: "YYYY-MM-DD ", "YYYY-MM-DD 123", "YYYY-MM-DD (BC)" 4. Any characters after trailing character 'T': "YYYY-MM-DDT" Below patterns are invalid: 1. Year is too large (exceed INT32_MAX): 20150318 2. Other separators "YYYY/MM/DD" Reference: Spark cast from string to date: https://github.com/apache/spark/blob/3e5203c64c06cc8a8560dfa0fb6f52e74589b583/sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala#L286-L298 Unit test: CastExprTest::fromStringToDate is derived from https://github.com/apache/spark/blob/3a9185964a0de3c720a6b77d38a446258b73468e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala#L103-L126 CastExprTest::fromStringToDateInvalid is derived from https://github.com/apache/spark/blob/3a9185964a0de3c720a6b77d38a446258b73468e/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOffSuite.scala#L67-L73 Pull Request resolved: facebookincubator#5844 Reviewed By: kevinwilfong Differential Revision: D48881690 Pulled By: bikramSingh91 fbshipit-source-id: 4236669585cf76762da1bc96d7976014aebab5d3

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 Jul 26, 2023

marin-ma force-pushed the date-pattern branch from 4f1bc59 to 48d4547 Compare August 1, 2023 05:43

marin-ma changed the title ~~Support more Date pattern~~ Support more date pattern for sparksql cast from string to date Aug 1, 2023

marin-ma force-pushed the date-pattern branch from 48d4547 to 0867095 Compare August 1, 2023 11:14

mbasmanova reviewed Aug 1, 2023

View reviewed changes

marin-ma force-pushed the date-pattern branch 2 times, most recently from 2cda6f8 to 2c5370e Compare August 2, 2023 10:37

mbasmanova reviewed Aug 2, 2023

View reviewed changes

marin-ma mentioned this pull request Aug 4, 2023

Add castFromDateString for different cast behavior #5994

Closed

marin-ma force-pushed the date-pattern branch from 9af4887 to 5297b72 Compare August 11, 2023 10:13

marin-ma force-pushed the date-pattern branch from 5297b72 to f26795a Compare August 15, 2023 04:02

bikramSingh91 reviewed Aug 22, 2023

View reviewed changes

bikramSingh91 mentioned this pull request Aug 22, 2023

Allow truncating fraction part in cast(string as integer) #6111

Closed

mbasmanova reviewed Aug 22, 2023

View reviewed changes

marin-ma force-pushed the date-pattern branch from 6b4646a to 289022c Compare August 24, 2023 02:50

update error message

3344f26

marin-ma force-pushed the date-pattern branch from 15246c2 to 3344f26 Compare August 31, 2023 11:01

fix

8e1bae7

bikramSingh91 reviewed Sep 5, 2023

View reviewed changes

jinchengchenghh reviewed Sep 6, 2023

View reviewed changes

linter

6b76f33

bikramSingh91 reviewed Sep 6, 2023

View reviewed changes

marin-ma mentioned this pull request Sep 7, 2023

Enable trailing spaces in standard mode string-to-date conversion #6468

Closed

remove presto

0fbc6a2

facebook-github-bot closed this in 8d6c296 Sep 15, 2023

facebook-github-bot added the Merged label Sep 15, 2023

		@@ -105,6 +105,8 @@ constexpr int32_t kCumulativeYearDays[] = {

		namespace {

		enum class ParseMode { STRICT, NON_STRICT, STANDARD_CAST, NON_STANDARD_CAST };

		@@ -270,6 +270,15 @@ bool tryParseDateString(

		// In standard-cast mode, no more trailing characters.

Support all Spark patterns in cast(varchar as date) #5844

Support all Spark patterns in cast(varchar as date) #5844

Conversation

marin-ma commented Jul 26, 2023 • edited Loading

netlify bot commented Jul 26, 2023 • edited Loading

✅ Deploy Preview for meta-velox ready!

netlify bot commented Jul 26, 2023

👷 Deploy Preview for meta-velox processing.

marin-ma commented Aug 1, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

marin-ma commented Aug 1, 2023

marin-ma commented Aug 1, 2023

mbasmanova commented Aug 1, 2023

kagamiori commented Aug 1, 2023

marin-ma commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marin-ma commented Aug 15, 2023

marin-ma commented Aug 17, 2023

marin-ma commented Aug 22, 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

Choose a reason for hiding this comment

marin-ma commented Aug 24, 2023

bikramSingh91 commented Aug 31, 2023

facebook-github-bot commented Aug 31, 2023

marin-ma commented Sep 1, 2023 • edited Loading

facebook-github-bot commented Sep 1, 2023

marin-ma commented Sep 5, 2023

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 Sep 6, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Sep 7, 2023

bikramSingh91 commented Sep 7, 2023

marin-ma commented Sep 11, 2023

marin-ma commented Sep 13, 2023

facebook-github-bot commented Sep 14, 2023

bikramSingh91 commented Sep 14, 2023

facebook-github-bot commented Sep 15, 2023

marin-ma commented Jul 26, 2023 •

edited

Loading

netlify bot commented Jul 26, 2023 •

edited

Loading

marin-ma commented Sep 1, 2023 •

edited

Loading