Fix CAST(tinyint/smallint/integer/bigint as varbinary) for Spark #9819

rui-mo · 2024-05-15T06:02:42Z

netlify · 2024-05-15T06:03:07Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`77eda91`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6650838991df410008b730b8

rui-mo · 2024-05-15T06:19:47Z

@PHILO-HE Would you like to take a review? Thanks.

velox/expression/CastExpr.cpp

velox/expression/PrestoCastHooks.cpp

velox/expression/CastExpr.cpp

velox/functions/sparksql/specialforms/SparkCastHooks.h

jinchengchenghh · 2024-05-16T12:08:37Z

Do you need to update the document?

rui-mo · 2024-05-23T07:53:16Z

@jinchengchenghh Updated the documentation. Thanks.

velox/expression/CastExpr.cpp

velox/functions/sparksql/specialforms/SparkCastHooks.h

velox/docs/functions/spark/conversion.rst

rui-mo · 2024-05-23T12:41:09Z

@mbasmanova Could you help review this change? Thanks!

mbasmanova

@rui-mo Looks great. Do we have Fuzzer coverage for this type of CAST?

mbasmanova · 2024-05-24T00:19:06Z

velox/docs/functions/spark/conversion.rst

@@ -184,3 +184,22 @@ Valid example
  SELECT cast(' -3E+2' as decimal(12, 2)); -- -300.00
  SELECT cast('-3E+2 ' as decimal(12, 2)); -- -300.00
  SELECT cast('  -3E+2  ' as decimal(12, 2)); -- -300.00
+
+Cast to Varbinary
+---------------


nit: please, extend to cover full length of the title

mbasmanova · 2024-05-24T00:26:45Z

velox/expression/CastExpr.cpp

+    const BaseVector& input) {
+  VELOX_USER_CHECK(
+      hooks_->canCastIntToBinary(),
+      "Cannot cast {} to VARBINARY.",


It seems too late for this type of check. Shouldn't we fail much earlier when compiling the expression and creating an instance of CastExpr?

CC: @kagamiori

Moved this check to constructSpecialForm which will be called by compileRewrittenExpression.

velox/velox/expression/ExprCompiler.cpp

Lines 408 to 409 in b842c14

result = specialForm->constructSpecialForm(

resultType, std::move(compiledInputs), trackCpuUsage, config);

mbasmanova · 2024-05-24T00:28:37Z

velox/expression/CastExpr.cpp

+  VectorPtr result;
+  context.ensureWritable(rows, VARBINARY(), result);
+  (*result).clearNulls(rows);


This can be simplified to

auto result = BaseVector::create<FlatVector<StringView>>(VARBINARY(), rows.end(), context.pool());

Thanks. Updated.

facebook-github-bot · 2024-05-24T00:29:55Z

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

rui-mo

Do we have Fuzzer coverage for this type of CAST?

@mbasmanova We need to add below signatures in getSignaturesForCast for this type.

for (auto fromType : {"tinyint", "smallint", "integer", "bigint"}) {
signatures.push_back(makeCastSignature(fromType, "varbinary"));
}

velox/velox/expression/tests/ExpressionFuzzer.cpp

Lines 125 to 126 in b842c14

    
           std::vector<facebook::velox::exec::FunctionSignaturePtr> 
        
           getSignaturesForCast() {

But this modification would cause Presto fuzzer test to fail because of this check #9819 (comment). I wonder if I can follow-up below TODO in a separate PR to provide custom cast signatures for Presto and Spark. Thanks.

velox/velox/expression/tests/ExpressionFuzzer.cpp

Lines 251 to 254 in b842c14

    
           "cast", 
        
           /// TODO: Add supported Cast signatures to CastTypedExpr and expose 
        
           /// them to fuzzer instead of hard-coding signatures here. 
        
           getSignaturesForCast(),

mbasmanova · 2024-05-24T09:30:54Z

@rui-mo

I wonder if I can follow-up below TODO in a separate PR to provide custom cast signatures for Presto and Spark.

That would be great. Thanks.

mbasmanova · 2024-05-24T09:32:24Z

velox/expression/CastExpr.cpp

+  VectorPtr result = BaseVector::create<FlatVector<StringView>>(
+      VARBINARY(), rows.end(), context.pool());
+  auto flatResult = result->asFlatVector<StringView>();


BaseVector::create<FlatVector<StringView>> returns FlatVectorPtr<StringView>. If you want VectorPtr, you can just call BaseVector::create

Thanks the the pointer. Updated.

mbasmanova · 2024-05-24T09:35:31Z

velox/expression/CastExpr.cpp

@@ -980,6 +1029,15 @@ ExprPtr CastCallToSpecialForm::constructSpecialForm(
    std::vector<ExprPtr>&& compiledChildren,
    bool trackCpuUsage,
    const core::QueryConfig& config) {
+  const auto inputKind = compiledChildren[0]->type()->kind();


Shouldn't this happen after checking that compiledChildren.size() == 1 on L1041?

Might be safer to do that in CastExpr's constructor.

Thanks for the catch.

Might be safer to do that in CastExpr's constructor.

SparkCastExpr extends CastExpr, so the check would impact Spark as well if it's in the constructor of CastExpr.

Got it. I thought you had an API in the CastHooks that tells whether this cast is supported or not. I see that you removed that API now.

facebook-github-bot · 2024-05-24T12:32:37Z

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

mbasmanova · 2024-05-24T13:40:26Z

@rui-mo I'm seeing "Conbench performance report — Found 2 regressions". Would you take a look?

CC: @assignUser @kgpai

facebook-github-bot · 2024-05-24T15:46:08Z

@mbasmanova merged this pull request in 9446f67.

conbench-facebook · 2024-05-24T16:23:55Z

Conbench analyzed the 1 benchmark run on commit 9446f67b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

rui-mo · 2024-05-27T00:30:08Z

@mbasmanova Thanks for your review. It appears that no regression is mentioned in the most recent report. Please kindly contact me if I'm missing something.

mbasmanova · 2024-05-28T13:04:07Z

@rui-mo Perf testing can be flaky at times. This must be one of these cases. CC: @assignUser

assignUser · 2024-05-28T22:27:42Z

@mbasmanova @rui-mo Yeah looks like at the time this was committed the times where very close together so false positives can happen. But while checking this out I noticed that PR a day later clearly made the results worse: https://velox-conbench.voltrondata.run/compare/benchmark-results/0665116962307f828000737782ce0ccc...06651355ec2377e8800073bd529feb0a/

…ebookincubator#9819) Summary: Fixes facebookincubator#9820. Pull Request resolved: facebookincubator#9819 Reviewed By: amitkdutta Differential Revision: D57756701 Pulled By: mbasmanova fbshipit-source-id: 954440314175b4eb9a9dcba553909d448babd935

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 May 15, 2024

rui-mo force-pushed the wip_to_binary branch from 632aad4 to dfbc361 Compare May 15, 2024 06:13

rui-mo changed the title ~~Fix cast(int as varbinary)~~ Fix CAST(int as varbinary) May 15, 2024

rui-mo mentioned this pull request May 15, 2024

[VL] Casting integer to binary type produces inconsistent result with Spark apache/incubator-gluten#5073

Open

rui-mo force-pushed the wip_to_binary branch from dfbc361 to 80f1c58 Compare May 15, 2024 06:20

PHILO-HE reviewed May 15, 2024

View reviewed changes

velox/expression/CastExpr.cpp Outdated Show resolved Hide resolved

velox/expression/PrestoCastHooks.cpp Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_to_binary branch 2 times, most recently from acab0aa to 59045ff Compare May 15, 2024 11:28

jinchengchenghh approved these changes May 16, 2024

View reviewed changes

velox/expression/CastExpr.cpp Outdated Show resolved Hide resolved

velox/functions/sparksql/specialforms/SparkCastHooks.h Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_to_binary branch 2 times, most recently from 1d4bbb0 to 2dae8eb Compare May 23, 2024 07:04

jinchengchenghh reviewed May 23, 2024

View reviewed changes

velox/expression/CastExpr.cpp Show resolved Hide resolved

velox/functions/sparksql/specialforms/SparkCastHooks.h Outdated Show resolved Hide resolved

velox/docs/functions/spark/conversion.rst Outdated Show resolved Hide resolved

rui-mo force-pushed the wip_to_binary branch 2 times, most recently from a6a213a to 6d1ecd0 Compare May 23, 2024 12:34

mbasmanova approved these changes May 24, 2024

View reviewed changes

mbasmanova reviewed May 24, 2024

View reviewed changes

mbasmanova 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 May 24, 2024

mbasmanova changed the title ~~Fix CAST(int as varbinary)~~ Fix CAST(tinyint/smallint/integer/bigint as varbinary) for Spark May 24, 2024

rui-mo force-pushed the wip_to_binary branch 2 times, most recently from 956cfbd to 9d05813 Compare May 24, 2024 06:50

rui-mo commented May 24, 2024

View reviewed changes

mbasmanova reviewed May 24, 2024

View reviewed changes

Fix Spark cast(int as binary)

77eda91

rui-mo force-pushed the wip_to_binary branch from 9d05813 to 77eda91 Compare May 24, 2024 12:09

facebook-github-bot closed this in 9446f67 May 24, 2024

facebook-github-bot added the Merged label May 24, 2024

rui-mo mentioned this pull request May 28, 2024

Add custom cast signatures for Presto and Spark fuzzer test #9949

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CAST(tinyint/smallint/integer/bigint as varbinary) for Spark #9819

Fix CAST(tinyint/smallint/integer/bigint as varbinary) for Spark #9819

rui-mo commented May 15, 2024 •

edited

Loading

netlify bot commented May 15, 2024 •

edited

Loading

rui-mo commented May 15, 2024

jinchengchenghh commented May 16, 2024

rui-mo commented May 23, 2024

rui-mo commented May 23, 2024

mbasmanova left a comment

mbasmanova May 24, 2024

mbasmanova May 24, 2024

rui-mo May 24, 2024 •

edited

Loading

mbasmanova May 24, 2024

rui-mo May 24, 2024

facebook-github-bot commented May 24, 2024

rui-mo left a comment

mbasmanova commented May 24, 2024

mbasmanova May 24, 2024

rui-mo May 24, 2024

mbasmanova May 24, 2024

rui-mo May 24, 2024

mbasmanova May 24, 2024

facebook-github-bot commented May 24, 2024

mbasmanova commented May 24, 2024

facebook-github-bot commented May 24, 2024

conbench-facebook bot commented May 24, 2024

rui-mo commented May 27, 2024

mbasmanova commented May 28, 2024

assignUser commented May 28, 2024

	result = specialForm->constructSpecialForm(
	resultType, std::move(compiledInputs), trackCpuUsage, config);

	std::vector<facebook::velox::exec::FunctionSignaturePtr>
	getSignaturesForCast() {

	"cast",
	/// TODO: Add supported Cast signatures to CastTypedExpr and expose
	/// them to fuzzer instead of hard-coding signatures here.
	getSignaturesForCast(),

Fix CAST(tinyint/smallint/integer/bigint as varbinary) for Spark #9819

Fix CAST(tinyint/smallint/integer/bigint as varbinary) for Spark #9819

Conversation

rui-mo commented May 15, 2024 • edited Loading

netlify bot commented May 15, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

rui-mo commented May 15, 2024

jinchengchenghh commented May 16, 2024

rui-mo commented May 23, 2024

rui-mo commented May 23, 2024

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

rui-mo May 24, 2024 • edited Loading

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 May 24, 2024

rui-mo left a comment

Choose a reason for hiding this comment

mbasmanova commented May 24, 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

facebook-github-bot commented May 24, 2024

mbasmanova commented May 24, 2024

facebook-github-bot commented May 24, 2024

conbench-facebook bot commented May 24, 2024

rui-mo commented May 27, 2024

mbasmanova commented May 28, 2024

assignUser commented May 28, 2024

rui-mo commented May 15, 2024 •

edited

Loading

netlify bot commented May 15, 2024 •

edited

Loading

rui-mo May 24, 2024 •

edited

Loading