Add tests for casting UNKNOWN type to any other type #6996

PHILO-HE · 2023-10-11T13:24:55Z

No description provided.

netlify · 2023-10-11T13:25:01Z

✅ Deploy Preview for meta-velox canceled.

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

PHILO-HE · 2024-02-12T02:55:24Z

@mbasmanova, could you spare some time to review this patch? Thanks!

mbasmanova

@PHILO-HE Curious, what's the motivation for this PR?

mbasmanova · 2024-02-12T10:48:42Z

velox/exec/tests/utils/QueryAssertions.cpp

@@ -274,6 +274,14 @@ variant variantAt<TypeKind::TIMESTAMP>(
      dataChunk->GetValue(column, row).GetValue<::duckdb::timestamp_t>()));
 }

+template <>
+variant variantAt<TypeKind::UNKNOWN>(


When would this be called? I believe there is a check for null before this call. If that's the case, then, perhaps, a safer implementation would be to throw VELOX_UNREACHABLE.

Just removed it and will cover it in another PR.

mbasmanova · 2024-02-12T10:49:21Z

velox/expression/CastExpr.cpp

@@ -724,6 +734,9 @@ void CastExpr::applyPeeled(
            fromType->asRow(),
            toType);
        break;
+      case TypeKind::UNKNOWN:
+        result = applyUnknown(rows, input, context, fromType, toType);


Why not just inline

result = BaseVector::createNullConstant(toType, rows.end(), context.pool());

mbasmanova · 2024-02-12T10:50:02Z

velox/expression/CastExpr.h

@@ -113,6 +113,13 @@ class CastExpr : public SpecialForm {
      const TypePtr& toType,
      VectorPtr& result);

+  VectorPtr applyUnknown(


This method is not using any member variables, hence, can be a free function in .cpp file. Also, the implementation is a one-liner and there is only one caller, hence, this function is not needed at all.

mbasmanova · 2024-02-12T10:51:34Z

velox/expression/tests/CastExprTest.cpp

@@ -371,6 +371,19 @@ TEST_F(CastExprTest, basics) {
      {"1.888", "2.5", "3.6", "100.44", "-100.101", "1", "-2"});
 }

+TEST_F(CastExprTest, fromUnknownType) {
+  testCast<UnknownValue, int>(


int -> int32_t

Can we test all types?

mbasmanova · 2024-02-12T10:51:47Z

velox/functions/prestosql/aggregates/PrestoHasher.cpp

+FOLLY_ALWAYS_INLINE void PrestoHasher::hash<TypeKind::UNKNOWN>(
+    const SelectivityVector& rows,
+    BufferPtr& hashes) {
+  applyHashFunction(rows, *vector_.get(), hashes, [&](auto row) { return 0; });


Would you add a test case for this change?

mbasmanova · 2024-02-12T10:53:35Z

velox/functions/prestosql/types/JsonType.cpp

+simdjson::error_code appendMapKey<TypeKind::UNKNOWN>(
+    const std::string_view& value,
+    exec::GenericWriter& writer) {
+  VELOX_NYI("UNKNOWN type is not supported!");


Will this ever be called? Isn't there a null check before this call? If so, VELOX_UNREACHABLE would be more appropriate.

mbasmanova · 2024-02-12T10:54:16Z

velox/type/Type.h

@@ -129,6 +129,10 @@ struct UnknownValue {
  bool operator>=(const UnknownValue& /* b */) const {
    return true;
  }
+
+  operator std::string() const {
+    return "NULL";


What is this change for? Would it be possible to add a test?

mbasmanova · 2024-02-12T10:54:49Z

velox/type/Type.h

@@ -1420,6 +1424,10 @@ std::shared_ptr<const OpaqueType> OPAQUE() {
        return TEMPLATE_FUNC<::facebook::velox::TypeKind::TIMESTAMP>(         \
            __VA_ARGS__);                                                     \
      }                                                                       \
+      case ::facebook::velox::TypeKind::UNKNOWN: {                            \
+        return TEMPLATE_FUNC<::facebook::velox::TypeKind::UNKNOWN>(           \


Do we need this change? It will require a lot of updates to existing code. Would it be possible to avoid it?

PHILO-HE · 2024-02-16T16:11:07Z

@PHILO-HE Curious, what's the motivation for this PR?

Hi @mbasmanova, thanks for your review! The motivation is we are trying to fix issues when enabling Spark NullType (UNKNOWN type of Velox).
I just divided this PR and removed the code irrelevant to the topic of this pr. Could you review again?

PHILO-HE · 2024-02-16T16:18:26Z

velox/type/Type.h

@@ -2390,6 +2390,16 @@ struct IsRowType<Row<Ts...>> {

 } // namespace facebook::velox

+namespace std {
+template <>
+struct hash<::facebook::velox::UnknownValue> {


The hash for UNKNOWN type is required by VectorMakerStats. If lacked, compiling will fail.
See https://github.com/facebookincubator/velox/blob/main/velox/vector/tests/utils/VectorMakerStats.h#L86.

VectorMakerStats is deprecated. It would be great to not use it (and eventually delete it).

I just removed this and introduced some code in VectorMakerStats.h to make the compiling pass. Maybe, better to delete deprecated code in a separate PR.

mbasmanova

Looks good % let's update documentation.

PHILO-HE · 2024-02-17T16:13:08Z

Hi @mbasmanova, I found the added tests can pass without any change for CastExpr.cpp. So I just removed the unused change and changed the PR title.

mbasmanova

@PHILO-HE Looks good % one nit.

mbasmanova · 2024-02-26T16:46:19Z

velox/docs/functions/spark/conversion.rst

@@ -2,6 +2,8 @@
 Conversion Functions
 ====================

+Casting from UNKNOWN type to all other scalar types are supported, e.g., cast(NULL as int).


typo: "Casting.. are supported" -> "Casting... is supported"

@mbasmanova, just fixed it and rebased the code. Thanks!

facebook-github-bot · 2024-02-26T16:47:10Z

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

facebook-github-bot · 2024-02-27T10:32:48Z

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

facebook-github-bot · 2024-02-27T10:33:18Z

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

facebook-github-bot · 2024-02-27T12:09:58Z

@mbasmanova merged this pull request in 62b8cfd.

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 11, 2023

PHILO-HE force-pushed the fix-unknown-type branch from bc36f5a to eaa0b75 Compare November 13, 2023 00:30

PHILO-HE force-pushed the fix-unknown-type branch from eaa0b75 to cddc99b Compare January 10, 2024 01:30

PHILO-HE marked this pull request as ready for review January 15, 2024 01:47

PHILO-HE force-pushed the fix-unknown-type branch from be9f06c to 7f546f2 Compare February 9, 2024 15:30

PHILO-HE changed the title ~~Support casting unknow type to other types~~ Support casting unknown type to other types Feb 11, 2024

PHILO-HE force-pushed the fix-unknown-type branch from d35bc61 to 214198b Compare February 11, 2024 14:22

PHILO-HE changed the title ~~Support casting unknown type to other types~~ Support casting UNKNOWN type to other types Feb 11, 2024

mbasmanova reviewed Feb 12, 2024

View reviewed changes

PHILO-HE force-pushed the fix-unknown-type branch 3 times, most recently from ff45670 to d4576d4 Compare February 16, 2024 16:03

PHILO-HE commented Feb 16, 2024

View reviewed changes

mbasmanova approved these changes Feb 16, 2024

View reviewed changes

mbasmanova changed the title ~~Support casting UNKNOWN type to other types~~ Add support to cast UNKNOWN type to any other type Feb 16, 2024

PHILO-HE changed the title ~~Add support to cast UNKNOWN type to any other type~~ Add tests for casting UNKNOWN type to any other type Feb 17, 2024

PHILO-HE mentioned this pull request Feb 20, 2024

[GLUTEN-2956][VL] Support Spark NullType apache/incubator-gluten#2996

Merged

mbasmanova reviewed Feb 26, 2024

View reviewed changes

PHILO-HE force-pushed the fix-unknown-type branch from 98f569a to 596a342 Compare February 27, 2024 01:18

PHILO-HE added 5 commits February 27, 2024 09:19

Initial commit

bb103f2

Remove useless change

6409ebb

Fix compile issue

44b2436

Update doc

a4c8679

Fix comment

596a342

facebook-github-bot closed this in 62b8cfd Feb 27, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for casting UNKNOWN type to any other type #6996

Add tests for casting UNKNOWN type to any other type #6996

PHILO-HE commented Oct 11, 2023 •

edited by mbasmanova

Loading

netlify bot commented Oct 11, 2023 •

edited

Loading

PHILO-HE commented Feb 12, 2024

mbasmanova left a comment

mbasmanova Feb 12, 2024

PHILO-HE Feb 16, 2024

mbasmanova Feb 12, 2024

mbasmanova Feb 12, 2024

mbasmanova Feb 12, 2024

mbasmanova Feb 12, 2024

mbasmanova Feb 12, 2024

mbasmanova Feb 12, 2024

mbasmanova Feb 12, 2024

PHILO-HE commented Feb 16, 2024

PHILO-HE Feb 16, 2024

mbasmanova Feb 16, 2024

PHILO-HE Feb 17, 2024

mbasmanova left a comment

PHILO-HE commented Feb 17, 2024

mbasmanova left a comment

mbasmanova Feb 26, 2024

PHILO-HE Feb 27, 2024

facebook-github-bot commented Feb 26, 2024

facebook-github-bot commented Feb 27, 2024

facebook-github-bot commented Feb 27, 2024

facebook-github-bot commented Feb 27, 2024

Add tests for casting UNKNOWN type to any other type #6996

Add tests for casting UNKNOWN type to any other type #6996

Conversation

PHILO-HE commented Oct 11, 2023 • edited by mbasmanova Loading

netlify bot commented Oct 11, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

PHILO-HE commented Feb 12, 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

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

PHILO-HE commented Feb 16, 2024

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

PHILO-HE commented Feb 17, 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

facebook-github-bot commented Feb 26, 2024

facebook-github-bot commented Feb 27, 2024

facebook-github-bot commented Feb 27, 2024

facebook-github-bot commented Feb 27, 2024

PHILO-HE commented Oct 11, 2023 •

edited by mbasmanova

Loading

netlify bot commented Oct 11, 2023 •

edited

Loading