Add isOrderable and isComparable for Type #6770

duanmeng · 2023-09-27T15:08:18Z

In Presto, input to min/max/array_sort is restricted to orderable types. For example, MAP type is not orderable. Velox currently allows MAP inputs.
This PR extends the Velox Type class to add isOrderable() and isComparable() methods.

Part of #6718 and #6712

netlify · 2023-09-27T15:08:26Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`4d02d8e`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65156d0b81539a0008590b9d

velox/type/Type.h

duanmeng · 2023-09-27T17:02:26Z

@mbasmanova Hi Masha, I've updated the PR with the logical type solution and added related tests, could you please help to take a look? Thanks a lot.

mbasmanova

Thanks.

velox/type/Type.cpp

velox/type/Type.h

facebook-github-bot · 2023-09-27T17:15:56Z

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

pedroerp

I think there's a small bug above.

velox/type/Type.cpp

pedroerp · 2023-09-27T17:23:56Z

velox/type/tests/TypeTest.cpp

+
+  rowType = ROW({INTEGER(), mapType});
+  EXPECT_FALSE(rowType->isOrderable());
+  EXPECT_FALSE(rowType->isComparable());


the tests here don't catch the bug I pointed out above. Maybe re-design the test so that isOrderable != isComparable?

@pedroerp Yes, the test case in line 883 is wrong (it was intended to test isOrderable != isComparable), I've updated it, thanks.

auto mapType = MAP(INTEGER(), REAL()); ... rowType = ROW({INTEGER(), mapType}); EXPECT_FALSE(rowType->isOrderable()); - EXPECT_FALSE(rowType->isComparable()); + EXPECT_TRUE(rowType->isComparable());

pedroerp · 2023-09-27T17:24:26Z

velox/type/Type.h

@@ -455,6 +455,10 @@ class Type : public Tree<const TypePtr>, public velox::ISerializable {

  virtual bool isPrimitiveType() const = 0;

+  virtual bool isOrderable() const = 0;


here would be the right place for a comment explaining what exactly these two methods should return, semantically.

velox/type/Type.cpp

duanmeng · 2023-09-28T05:06:05Z

@mbasmanova @pedroerp @bikramSingh91 I've resolved all the comments added comments for the two APIs, and updated the test cases to guard the difference between the two APIs. Could you please help to take a look? Thanks.

mbasmanova

@duanmeng Thank you for iterating. Looks great % one comment.

mbasmanova · 2023-09-28T11:43:19Z

velox/type/Type.h

@@ -455,6 +455,16 @@ class Type : public Tree<const TypePtr>, public velox::ISerializable {

  virtual bool isPrimitiveType() const = 0;

+  /// Return true if the type is scalar type or unknown type. Return false if


This is a virtual method and therefore it cannot say what its implementations will return. Instead in need to explain what the return value means. How about,

Returns true if equality relationship is defined for the values of this type, i.e. a == b is defined and returns true, false or null. For example, scalar types are usually comparable and complex types are comparable if their nested types are.

Got it, updated.

mbasmanova · 2023-09-28T11:44:06Z

velox/type/Type.h

+  /// if all its children types are comparable.
+  virtual bool isComparable() const = 0;
+
+  /// Return true if the type is scalar type or unknown type. Return false if


ditto

Returns true if less than relationship is defined for the values of this type, i.e. a <= b returns true or false. For example, scalar types are usually orderable, arrays and structs are orderable if their nested types are, while map types are not orderable.

facebook-github-bot · 2023-09-28T11:46:10Z

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

mbasmanova

Thanks.

facebook-github-bot · 2023-09-28T12:30:03Z

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

facebook-github-bot · 2023-09-29T01:07:02Z

@mbasmanova merged this pull request in b661e8c.

…n signature (#6906) Summary: In Presto, input to min/max is restricted to orderable types. For example, MAP type is not orderable. Velox currently allows MAP inputs. This PR adds two APIs in `FunctionSignatureBuilder`: `orderableTypeVariable` and `comparableTypeVariable`. Use type's `orderable` and `comparable` flag added in #6770 to do the check during type binding in `SignatureBinder.tryBind`. Part of #6718 Pull Request resolved: #6906 Reviewed By: xiaoxmeng Differential Revision: D49951511 Pulled By: mbasmanova fbshipit-source-id: 17ee5fd75b25a7df636279d07e99773105349220

Summary: In Presto, input to min/max/array_sort is restricted to orderable types. For example, MAP type is not orderable. Velox currently allows MAP inputs. This PR extends the Velox Type class to add `isOrderable()` and `isComparable()` methods. Part of facebookincubator#6718 and facebookincubator#6712 Pull Request resolved: facebookincubator#6770 Reviewed By: Yuhta Differential Revision: D49693596 Pulled By: mbasmanova fbshipit-source-id: 1386904dde45691368e759831b4bf24287b0de5d

…n signature (facebookincubator#6906) Summary: In Presto, input to min/max is restricted to orderable types. For example, MAP type is not orderable. Velox currently allows MAP inputs. This PR adds two APIs in `FunctionSignatureBuilder`: `orderableTypeVariable` and `comparableTypeVariable`. Use type's `orderable` and `comparable` flag added in facebookincubator#6770 to do the check during type binding in `SignatureBinder.tryBind`. Part of facebookincubator#6718 Pull Request resolved: facebookincubator#6906 Reviewed By: xiaoxmeng Differential Revision: D49951511 Pulled By: mbasmanova fbshipit-source-id: 17ee5fd75b25a7df636279d07e99773105349220

duanmeng requested a review from mbasmanova September 27, 2023 15:08

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

mbasmanova requested a review from laithsakka September 27, 2023 15:44

mbasmanova reviewed Sep 27, 2023

View reviewed changes

velox/type/Type.h Show resolved Hide resolved

duanmeng force-pushed the typeFiled branch 2 times, most recently from 2041291 to 3d968d5 Compare September 27, 2023 16:54

Add isOrderable and isComparable

9fa7fea

duanmeng force-pushed the typeFiled branch from 3d968d5 to 9fa7fea Compare September 27, 2023 17:01

mbasmanova approved these changes Sep 27, 2023

View reviewed changes

velox/type/Type.cpp Outdated Show resolved Hide resolved

velox/type/Type.cpp Outdated Show resolved Hide resolved

velox/type/Type.h Outdated Show resolved Hide resolved

pedroerp requested changes Sep 27, 2023

View reviewed changes

bikramSingh91 reviewed Sep 27, 2023

View reviewed changes

velox/type/Type.cpp Outdated Show resolved Hide resolved

duanmeng force-pushed the typeFiled branch 3 times, most recently from 9894314 to 8b4af07 Compare September 28, 2023 04:56

mbasmanova reviewed Sep 28, 2023

View reviewed changes

resolve comments

4d02d8e

duanmeng force-pushed the typeFiled branch from 8b4af07 to 4d02d8e Compare September 28, 2023 12:09

mbasmanova reviewed Sep 28, 2023

View reviewed changes

facebook-github-bot closed this in b661e8c Sep 29, 2023

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

This was referenced Sep 30, 2023

Input type for min/max/min_by/max_by should be restricted to "orderable" types #6718

Closed

Allow restricting arguments as orderable or comparable in the function signature #6906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isOrderable and isComparable for Type #6770

Add isOrderable and isComparable for Type #6770

duanmeng commented Sep 27, 2023 •

edited

netlify bot commented Sep 27, 2023 •

edited

duanmeng commented Sep 27, 2023

mbasmanova left a comment

facebook-github-bot commented Sep 27, 2023

pedroerp left a comment

pedroerp Sep 27, 2023

duanmeng Sep 28, 2023 •

edited

pedroerp Sep 27, 2023

duanmeng Sep 28, 2023

duanmeng commented Sep 28, 2023

mbasmanova left a comment

mbasmanova Sep 28, 2023

duanmeng Sep 28, 2023

mbasmanova Sep 28, 2023

duanmeng Sep 28, 2023

facebook-github-bot commented Sep 28, 2023

mbasmanova left a comment

facebook-github-bot commented Sep 28, 2023

facebook-github-bot commented Sep 29, 2023

		@@ -455,6 +455,10 @@ class Type : public Tree<const TypePtr>, public velox::ISerializable {

		virtual bool isPrimitiveType() const = 0;

		virtual bool isOrderable() const = 0;

		@@ -455,6 +455,16 @@ class Type : public Tree<const TypePtr>, public velox::ISerializable {

		virtual bool isPrimitiveType() const = 0;

		/// Return true if the type is scalar type or unknown type. Return false if

Add isOrderable and isComparable for Type #6770

Add isOrderable and isComparable for Type #6770

Conversation

duanmeng commented Sep 27, 2023 • edited

netlify bot commented Sep 27, 2023 • edited

✅ Deploy Preview for meta-velox canceled.

duanmeng commented Sep 27, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 27, 2023

pedroerp left a comment

Choose a reason for hiding this comment

pedroerp Sep 27, 2023

Choose a reason for hiding this comment

duanmeng Sep 28, 2023 • edited

Choose a reason for hiding this comment

pedroerp Sep 27, 2023

Choose a reason for hiding this comment

duanmeng Sep 28, 2023

Choose a reason for hiding this comment

duanmeng commented Sep 28, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova Sep 28, 2023

Choose a reason for hiding this comment

duanmeng Sep 28, 2023

Choose a reason for hiding this comment

mbasmanova Sep 28, 2023

Choose a reason for hiding this comment

duanmeng Sep 28, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Sep 28, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 28, 2023

facebook-github-bot commented Sep 29, 2023

duanmeng commented Sep 27, 2023 •

edited

netlify bot commented Sep 27, 2023 •

edited

duanmeng Sep 28, 2023 •

edited