Add 'equivalent' method to Type #1740

majetideepak · 2022-05-31T15:18:42Z

The kindEquals method only checks if the TypeKind of two types matches recursively.
However, certain types like DecimalType and FixedSizedArrayType require their parameters
to match as well. This type of equivalence is needed by the SignatureBinder.
A new equivalent method has been added to Type for this requirement.
Example: Two FixedSizedArrayTypes are equivalent only if their lengths are equal.
Two DecimalTypes are equivalent only if their precision/scale are the same.

aditi-pandit · 2022-05-31T17:19:22Z

velox/type/tests/TypeTest.cpp

@@ -134,6 +134,9 @@ TEST(TypeTest, shortDecimal) {
  } catch (const VeloxRuntimeError& e) {
    EXPECT_EQ("(19 vs. 18)", e.message());
  }
+  EXPECT_TRUE(SHORT_DECIMAL(10, 5)->kindEquals(SHORT_DECIMAL(10, 5)));


Not related to this PR, but in the above tests we should try to construct a SHORT_DECIMAL with negative precision and scale and throw exception in those cases.

karteekmurthys · 2022-05-31T17:37:50Z

LGTM

mbasmanova

@majetideepak Deepak, what's the motivation for this change? Type::kindEquals is expected to check only type kinds (recursively), not type parameters. For example, it doesn't check len parameter for the FixedSizeArrayType.

majetideepak · 2022-06-06T20:00:57Z

@mbasmanova I was exploring decimal signatures and felt this change was required. For example:
For the signature below

auto signature = exec::FunctionSignatureBuilder()
                         .typeVariable("T")
                         .returnType("boolean")
                         .argumentType("array(T)")
                         .argumentType("T")
                         .build();

I would expect the following to fail since the argument types should not bind. But it doesn't since we use kindEquals in the SignatureBinder. I expect a similar problem for FixedSizeArrayType also.

testSignatureBinder(signature, {ARRAY(DECIMAL(20, 7)), DECIMAL(20, 8)}, BOOLEAN());

mbasmanova · 2022-06-06T20:47:01Z

@majetideepak Deepak, thank you for the context. Should we than rename the kindEquals method and change the comment to clarify that we are comparing everything (kind + parameters) except for the names? Also, should we fix the FixedSizeArrayType as well? Finally, is this a good idea to hard-code all these types in the implementation or add a virtual method to the Type?

majetideepak · 2022-06-06T21:15:13Z

@mbasmanova Yes to all your suggestions including adding a virtual method to Type. I will make another pass. Thanks for the feedback.

majetideepak · 2022-06-07T16:27:35Z

@mbasmanova I renamed kindEquals to equals. equals is now a pure virtual function.== is changed to virtual since it is the same as equals for most types except for ROW, Opaque types.
FixedSizeArrayType is fixed as well.

mbasmanova · 2022-06-07T19:44:26Z

@majetideepak Look good to me. I wonder if equals is a bit too similar to == so users won't know the difference and assume these are exactly the same. Would it make sense to come up with a name that clearly says that "equals" ignores the names of the children. Also, would you update the PR title and write up a description?

majetideepak · 2022-06-07T20:19:36Z

@mbasmanova for OpaqueTypes, equals is different from == where the typeIndex_ is not checked in equals. So equals is not just ignoring the names of children.
Does the name isSimilar sound better?

mbasmanova · 2022-06-07T20:29:45Z

Does the name isSimilar sound better?

Why not use "equivalent" term you used in the PR description?

facebook-github-bot · 2022-06-07T21:33:45Z

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

mbasmanova · 2022-06-07T21:35:17Z

@majetideepak Deepak, there are circle CI failures. Would you take a look?

majetideepak · 2022-06-07T23:43:22Z

@mbasmanova, CI is green.

facebook-github-bot · 2022-06-07T23:49:19Z

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

mbasmanova · 2022-06-08T13:36:17Z

@majetideepak Deepak, there are a lot of places that I need to change to accommodate this PR. As I'm changing these I'm having second thoughts. It feels like there is a clear use case for kindEquals method, e.g the function that operates on certain types can check if it got these. For example, a function that takes only decimals, can check if it got a short or long decimal. It wouldn't care what the precision and scales are. The use case for equivalence check is not as clear and I wonder if it actually makes sense to keep the kindEquals method that checks only the TypeKind recursively and add an equivalence method if there is a use case for it. What do you think?

majetideepak · 2022-06-08T14:51:27Z

@mbasmanova Let's take prestosql/ArrayContains.cpp that is modified in this PR to use equivalent.

    VELOX_CHECK(arrayVector->type()->asArray().elementType()->equivalent(
        *searchVector->type()));

If the type is decimal, it is not enough to check if arrayVector type and searchVector type are both ShortDecimal. We must verify that the precision and scale are also the same, for the operation to be meaningful.
I see your point that the ArrayContains function body only deals with native types, so kindEquals should be sufficient.

I feel we must also ensure that the operation is meaningful somewhere. Does it make sense then only for the SignatureBinder to use an equivalent method?

majetideepak · 2022-06-08T15:05:22Z

The SignatureBinder verifies the argument types. I also don't see why we need these checks in the functions.

mbasmanova · 2022-06-08T15:06:10Z

Does it make sense then only for the SignatureBinder to use an equivalent method?

This is a good question. At this moment, I do not see any other use case.

mbasmanova · 2022-06-08T16:02:54Z

velox/type/Type.h

@@ -471,7 +471,19 @@ class Type : public Tree<const std::shared_ptr<const Type>>,

  virtual std::string toString() const = 0;

-  virtual bool operator==(const Type& other) const = 0;
+  // Types are weakly matched.


Use /// for method and class-level comments.

mbasmanova · 2022-06-08T16:03:36Z

velox/type/Type.h

@@ -492,10 +504,10 @@ class Type : public Tree<const std::shared_ptr<const Type>>,

  static std::shared_ptr<const Type> create(const folly::dynamic& obj);

-  // recursive kind hashing (ignores names)
+  // recursive kind hashing (uses only typeKind)


nit: perhaps, fix the comment to start with a capital letter and end with a period. Same below.

facebook-github-bot · 2022-06-08T17:50:29Z

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

Summary: The `kindEquals` method only checks if the `TypeKind` of two types matches recursively. However, certain types like DecimalType and FixedSizedArrayType require their parameters to match as well. This type of equivalence is needed by the SignatureBinder. A new `equivalent` method has been added to Type for this requirement. Example: Two FixedSizedArrayTypes are equivalent only if their lengths are equal. Two DecimalTypes are equivalent only if their precision/scale are the same. Pull Request resolved: facebookincubator#1740 Reviewed By: pedroerp Differential Revision: D36986040 Pulled By: mbasmanova fbshipit-source-id: 495cbe1c01aa87aea47c1c9d4f891e3cc784f4b2

…Clickhouse successfully (facebookincubator#1740) What changes were proposed in this pull request? (Fixes: facebookincubator#1738) How was this patch tested? manual tests, run ep/build-clickhouse/src/build_clickhouse.sh

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 31, 2022

majetideepak requested review from karteekmurthys and mbasmanova May 31, 2022 15:18

aditi-pandit reviewed May 31, 2022

View reviewed changes

mbasmanova reviewed Jun 6, 2022

View reviewed changes

majetideepak force-pushed the support-decimal-equals branch from 6cc1be5 to 1094dcf Compare June 7, 2022 16:24

majetideepak force-pushed the support-decimal-equals branch 3 times, most recently from 44c5c29 to 345fb3e Compare June 7, 2022 17:45

majetideepak changed the title ~~Support Decimal kindEquals~~ Add equals method to Type Jun 7, 2022

majetideepak changed the title ~~Add equals method to Type~~ Add equals method to Type and remove kindEquals Jun 7, 2022

majetideepak changed the title ~~Add equals method to Type and remove kindEquals~~ Add equivalent method to Type and remove kindEquals Jun 7, 2022

majetideepak changed the title ~~Add equivalent method to Type and remove kindEquals~~ Add 'equivalent' method to Type and remove 'kindEquals' Jun 7, 2022

majetideepak force-pushed the support-decimal-equals branch 3 times, most recently from db5c1cc to 17aab6f Compare June 7, 2022 21:32

mbasmanova approved these changes Jun 7, 2022

View reviewed changes

mbasmanova requested review from zzhao0 and Yuhta and removed request for karteekmurthys June 7, 2022 21:33

majetideepak force-pushed the support-decimal-equals branch from 17aab6f to e00f9d3 Compare June 7, 2022 22:14

majetideepak added 2 commits June 8, 2022 11:27

Add equivalent function to Type

0b8034d

use equivalent inside SignatureBinder

96f501b

majetideepak force-pushed the support-decimal-equals branch from e00f9d3 to 96f501b Compare June 8, 2022 15:54

majetideepak changed the title ~~Add 'equivalent' method to Type and remove 'kindEquals'~~ Add 'equivalent' method to Type Jun 8, 2022

mbasmanova reviewed Jun 8, 2022

View reviewed changes

Fix comments

bc9998e

facebook-github-bot closed this in f8b3230 Jun 8, 2022

majetideepak deleted the support-decimal-equals branch June 16, 2022 18:36

majetideepak restored the support-decimal-equals branch June 16, 2022 18:36

majetideepak deleted the support-decimal-equals branch June 16, 2022 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'equivalent' method to Type #1740

Add 'equivalent' method to Type #1740

majetideepak commented May 31, 2022 •

edited

Loading

aditi-pandit May 31, 2022

karteekmurthys commented May 31, 2022

mbasmanova left a comment

majetideepak commented Jun 6, 2022

mbasmanova commented Jun 6, 2022

majetideepak commented Jun 6, 2022

majetideepak commented Jun 7, 2022 •

edited

Loading

mbasmanova commented Jun 7, 2022

majetideepak commented Jun 7, 2022 •

edited

Loading

mbasmanova commented Jun 7, 2022

facebook-github-bot commented Jun 7, 2022

mbasmanova commented Jun 7, 2022

majetideepak commented Jun 7, 2022

facebook-github-bot commented Jun 7, 2022

mbasmanova commented Jun 8, 2022

majetideepak commented Jun 8, 2022 •

edited

Loading

majetideepak commented Jun 8, 2022

mbasmanova commented Jun 8, 2022

mbasmanova Jun 8, 2022

mbasmanova Jun 8, 2022

facebook-github-bot commented Jun 8, 2022

Add 'equivalent' method to Type #1740

Add 'equivalent' method to Type #1740

Conversation

majetideepak commented May 31, 2022 • edited Loading

aditi-pandit May 31, 2022

Choose a reason for hiding this comment

karteekmurthys commented May 31, 2022

mbasmanova left a comment

Choose a reason for hiding this comment

majetideepak commented Jun 6, 2022

mbasmanova commented Jun 6, 2022

majetideepak commented Jun 6, 2022

majetideepak commented Jun 7, 2022 • edited Loading

mbasmanova commented Jun 7, 2022

majetideepak commented Jun 7, 2022 • edited Loading

mbasmanova commented Jun 7, 2022

facebook-github-bot commented Jun 7, 2022

mbasmanova commented Jun 7, 2022

majetideepak commented Jun 7, 2022

facebook-github-bot commented Jun 7, 2022

mbasmanova commented Jun 8, 2022

majetideepak commented Jun 8, 2022 • edited Loading

majetideepak commented Jun 8, 2022

mbasmanova commented Jun 8, 2022

mbasmanova Jun 8, 2022

Choose a reason for hiding this comment

mbasmanova Jun 8, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Jun 8, 2022

majetideepak commented May 31, 2022 •

edited

Loading

majetideepak commented Jun 7, 2022 •

edited

Loading

majetideepak commented Jun 7, 2022 •

edited

Loading

majetideepak commented Jun 8, 2022 •

edited

Loading