Add Spark cast expr based on cast hooks

[rui-mo]

There are several corner cases of semantic differences on cast between Presto
and Spark. According to the implementation of
registerFunctionCallToSpecialForm, a previously registered special form can be
overriden by one registered after it and of the same name.This PR implements
SparkCastExpr, which is registered as special form, and can be customized with
the help of cast hooks compatible with Spark's semantics. Below hooks are added
to solve several semantic differences.

• castStringToTimestamp
• castStringToDate
• castTimestampToString
• legacy
• removeWhiteSpaces
• truncate

Two configurations kCastToIntByTruncate and kCastStringToDateIsIso8601 are
replaced by cast hooks. These configurations are no longer used and will be removed
by subsequent PRs.

#4876
Fixes #8121.

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	fa86d9c
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/659df24130b28f0008712f1c

[mbasmanova]

@rui-mo Rui, overall I think it makes sense to Velox to allow applications to provide custom implementations for special forms and register new special forms as well. We need to figure out how to add proper APIs to do that.

Overriding CAST requires also customizing this code in ExprCompiler. CC: @bikramSingh91

  } else if (auto cast = dynamic_cast<const core::CastTypedExpr*>(expr.get())) {
    VELOX_CHECK(!compiledInputs.empty());
    if (FOLLY_UNLIKELY(*resultType == *compiledInputs[0]->type())) {
      result = compiledInputs[0];
    } else {
      result = std::make_shared<CastExpr>(
          resultType,
          std::move(compiledInputs[0]),
          trackCpuUsage,
          cast->nullOnFailure());
    }

[rui-mo]

@mbasmanova Thank you for the review. Can I prototype with kCastStringToDateIsIso8601 in this PR? That is to move the Spark-specific behavior controlled by this config into SparkCastExpr, and remove this config.

[mbasmanova]

@rui-mo Rui, please, go ahead.

[rui-mo]

@mbasmanova Masha, I made the prototype with IsIso8601. Could you help review? Thanks.

Overriding CAST requires also customizing this code in ExprCompiler.

Seems it is not necessary if we don't want to parse cast from string. In this PR, SparkCastExpr can to be created with CallTypedExpr, but if we want to parse from string like cast (c0 as int), more changes are required. Besides what is included in f8eb923, we need to decide whether to create CastExpr or SparkCastExpr in parseCastExpr (maybe through a parse option).

In this PR, parsing from string to SparkCastExpr is not included. But if it is needed, I can continue to work on that. Seems it is also a UT conveniency issue.

[mbasmanova]

@rui-mo Thank you for prototyping Spark-specific CAST. Overall looks good. A few things that need to be taken care of.

• Move SparkCastExpr out of velox/expression. Perhaps, into functions/sparksql/specialforms folder I saw you adding in some other PR.
• Make sure that CastTypedExpr is equivalent to CallTypedExpr("cast"). Currently, these are not the same because ExprCompiler explicitly converts CastTypedExpr to CastExpr. Perhaps, this logic can be replaced with getSpecialForm("cast").
• Figure out all the differences between Spark and Presto CAST impementations to see whether it is feasible to continue using CastExpr as a base class.

[rui-mo]

@mbasmanova Thank you for the suggestions. Fixed them, and apology for the delay.

Figure out all the differences between Spark and Presto CAST impementations to see whether it is feasible to continue using CastExpr as a base class.

Summarized them in this table: #4876 (comment).

[mbasmanova]

Summarized them in this table: #4876 (comment).

@rui-mo Thank you, Rui. What's your assessment of these differences? Is it feasible to continue using CastExpr as a base class?

[rui-mo]

What's your assessment of these differences? Is it feasible to continue using CastExpr as a base class?

@mbasmanova These cases can be divided into below categories.

1. cast_to_int_by_truncate behaviors.
2. overflow when casting to integral types.
3. cast from string to other type.
4. cast from other type to string.

As discussed with @PHILO-HE, we tend to use CastExpr as a base class, which can help reuse existing code and avoid duplicates. Take below case as an example.

Conversion	Cases	Presto result	Spark result
string -> integral/floating-point/decimal/date/timestamp	SELECT cast('\t\n\u001F 123\u000B' as int);	invalid	123

we can fix some of them by overriding a getCastFromStringOutput method of the base class, and removes the white-spaces (which is required in Spark for cast from string) before calling the cast function.

velox/velox/expression/CastExpr-inl.h

Lines 232 to 233 in 261d0b0

	auto output = util::Converter<ToKind, void, Truncate, LegacyCast>::cast(
	inputRowValue);

As for #5307 (comment), we don't need to implement duplicate cast functions in CastExpr and SparkCastExpr. By overriding a small part of code in SparkCastExpr to remove the white-spaces before cast should be enough.

Glad to hear your opinions, thanks.

[mbasmanova]

@rui-mo Rui, overall looks good. I have some question about the design: separate hook class vs. virtual methods in CastExpr. I also wonder how many hooks we would need to fully implement Spark semantics. Would it make sense to prototype this to see what it looks like and whether the set of hooks is managable.

[mbasmanova]

@rui-mo Rui, thank you for iterating on this. I'm having a few questions about the hooks API.

[facebook-github-bot]

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

[kagamiori]

FYI, I replace

static void SetUpTestCase() {
    functions::sparksql::registerFunctions("");
  }

in the SparkCastExprTest class with

SparkCastExprTest() {
    functions::sparksql::registerFunctions("");
  }

to resolve some internal test failures. Also made some format changes.

[facebook-github-bot]

@kagamiori merged this pull request in 51dc97b.

[conbench-facebook]

Conbench analyzed the 1 benchmark run on commit 51dc97b1.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[kagamiori]

@rui-mo, this change turns out to break some string-related unit test in spark, and hence the linux build job in CI (https://app.circleci.com/pipelines/github/facebookincubator/velox/42248/workflows/18d750f7-eed8-466f-94d8-63958efad94a/jobs/291869/tests), so we're going to revert it. Could you take a look at the failed unit tests?

[rui-mo]

@kagamiori Thank you for helping land this PR. I know the cause of this failure, and please check #8121. Could you tell me more details about the internal failures as #7377 (comment) mentioned?

[kagamiori]

@kagamiori Thank you for helping land this PR. I know the cause of this failure, and please check #8121. Could you tell me more details about the internal failures as #7377 (comment) mentioned?

Hi @rui-mo, we're seeing the CI signal "ci/circleci: linux-build" started failing on the main branch since #7377 was landed (https://github.com/facebookincubator/velox/commits/main/).

cc @kgpai

[rui-mo]

we're seeing the CI signal "ci/circleci: linux-build" started failing on the main branch

Hi @kagamiori, I checked the log and found StringTest.sha1 is failing. As I previously found, it is due to #8123 (comment). Could you tell me more about why SetUpTestCase() is removed and replaced as #7377 (comment)? Thanks.

[kagamiori]

@kagamiori Thank you for helping land this PR. I know the cause of this failure, and please check #8121. Could you tell me more details about the internal failures as #7377 (comment) mentioned?

OK, after reading the context in #8121, I think I know the reason to the failure. It was likely the replacement of SetUpTestCase() with SparkCastExprTest(). This change however didn't break the internal test of velox_functions_spark_test. Let me try making a fix.

cc @kgpai @mbasmanova

[kagamiori]

static void SetUpTestCase() {
functions::sparksql::registerFunctions("");
}

SetUpTestCase() was replaced because parse::registerTypeResolver(); and memory::MemoryManager::testingSetInstance({}); needs to be called additionally to avoid breaking internal test signals. Those were called in FunctionBaseTest::SetUpTestCase(), but when SparkCastExprTest::SetUpTestCase() is defined, the method in parent class wouldn't be called. I'm adding SparkCastExprTest::SetUpTestCase() back and adding these two calls to it.

[rui-mo]

@kagamiori Thank you. I understand. In this PR SetUpTestCase() is added to avoid the registration of Presto functions, but another two functions calls are missing. Sorry for the inconvenience, and please feel free to contact me if any other work is needed.

[kagamiori]

Here is a fix: #8346.

[rui-mo]

@mbasmanova @kagamiori Thanks for your continuous support on this pull request. I'm working on below tasks discovered when implementing Spark cast. Could you spare some time to take a review? Thanks!

1. Remove deprecated configs kCastToIntByTruncate and kCastStringToDateIsIso8601 #8352
2. Combine testComplexCast with testCast #8254
   Mentioned in Add Spark cast expr based on cast hooks #7377 (comment).
3. Optimize CAST(timestamp as varchar) #8385
   Mentioned in Add Spark cast expr based on cast hooks #7377 (comment).