Add ContainerRowSerde::compareWithNulls API #6419

duanmeng · 2023-09-05T12:27:47Z

Presto aggregate functions that compare values of complex types need to detect
cases when complex type values contain nulls. The new
ContainerRowSerde::compareWithNulls API allows to signal the case when complex
type value contains a null.

Part of #6314

netlify · 2023-09-05T12:27:52Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`952a601`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6500584cd5f5070008b6e28c

velox/exec/ContainerRowSerde.h

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

velox/exec/tests/ContainerRowSerdeTest.cpp

@@ -144,5 +145,91 @@ TEST_F(ContainerRowSerdeTest, nested) {
  testRoundTrip(nestedArray);
 }

+TEST_F(ContainerRowSerdeTest, compareNullsInRowVector) {
+  std::optional<std::optional<int>> x =


int -> int32_t

Is this variables used? If so, let's come up with a more descriptive name.

mbasmanova · 2023-09-11T11:44:38Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+  std::optional<std::optional<int>> x =
+      std::make_optional<std::optional<int>>(std::nullopt);
+  auto data = makeRowVector({
+      makeNullableFlatVector<int32_t>({


makeFlatVector

mbasmanova · 2023-09-11T12:02:43Z

velox/exec/ContainerRowSerde.cpp

  auto result = compareStringAsc(left, right, index, flags.equalsOnly);
  return flags.ascending ? result : result * -1;
 }

+std::optional<std::optional<int32_t>>
+compareNulls(bool leftNull, bool rightNull, CompareFlags flags) {


Is this a copy of BaseVector::compareNulls? If so, maybe change BaseVector::compareNulls to be a static public method and re-use.

Yes, we could reuse BaseVector::compareNulls. Updated.

mbasmanova · 2023-09-11T12:03:52Z

velox/exec/ContainerRowSerde.cpp

  auto result = compareStringAsc(left, right, index, flags.equalsOnly);
  return flags.ascending ? result : result * -1;
 }

+std::optional<std::optional<int32_t>>


Double optional is a bit hard to reason about. Is this necessary?

Reuse BaseVector::compareNulls now.

mbasmanova · 2023-09-11T12:06:01Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+  DecodedVector decodedVector;
+  decodedVector.decode(*rowVector, rows);
+
+  static const CompareFlags kCompareFlags{


It is confusing that we serializes a value of type ROW(INTEGER, VARCHAR), but compare it to a value of type ROW(INTEGER).

velox/exec/tests/ContainerRowSerdeTest.cpp

mbasmanova · 2023-09-11T12:07:14Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+  ByteStream out;
+  HashStringAllocator::prepareRead(position.header, out);
+
+  auto arrayVector = makeNullableArrayVector<int64_t>({


Would you also add tests for maps?

mbasmanova

@laithsakka Laith, would you help review this PR?

kagamiori

Hi @duanmeng, Thank you for adding this support to fix min() and max() functions! I left two questions.

kagamiori · 2023-09-11T17:06:46Z

velox/exec/ContainerRowSerde.h

@@ -44,6 +44,15 @@ class ContainerRowSerde {
      const Type* type,
      CompareFlags flags);

+  /// Return std::nullopt if encountered in NullHandlingMode::StopAtNull mode.
+  /// Support NullHandlingMode::NoStop mode as well and return
+  /// std::optional<int32_t>(0) if null encountered.


I may have missed this, but where do we return std::nullopt in the leaf cases?

I'm not sure I understand your question about ** leaf cases**.

It will return std::nullopt, say when an element of an ArrayVector is null during in StopAtAtNull mode. BaseVector::compareNulls returns std::nullopt in StopAtAtNull mode when a null is encountered.

std::optional<int32_t> compareArrays( ... if (leftNull || rightNull) { auto result = BaseVector::compareNulls(leftNull, rightNull, flags); if (result && *result == 0) { continue; } return result; }

/// Return std::nullopt if encountered in NullHandlingMode::StopAtNull mode.
/// Support NullHandlingMode::NoStop mode as well and return
/// std::optional<int32_t>(0) if null encountered.

This is not clear and may be wrong. I will update it as follows,
"Return std::nullopt if encountered in NullHandlingMode::StopAtNull mode. And return std::optional<int32_t>(0) if both sides are nulls in NullHandlingMode::NoStop mode."

kagamiori · 2023-09-11T17:08:21Z

velox/exec/ContainerRowSerde.cpp

  auto map = right.wrappedVector()->asUnchecked<MapVector>();
  VELOX_CHECK_EQ(map->encoding(), VectorEncoding::Simple::MAP);
  auto wrappedIndex = right.wrappedIndex(index);
  auto size = map->sizeAt(wrappedIndex);
  std::vector<vector_size_t> indices(size);
  auto rightIndices = map->sortedKeyIndices(wrappedIndex);
  auto result = compareArrayIndices(left, *map->mapKeys(), rightIndices, flags);
-  if (result) {
+  if (result.has_value() && result.value() != 0) {


nit: if (result && *result != 0) for consistency with other places?

Thanks for your remind. I've fixed it using the following code, and improve the ut of MapType.

if (result && *result == 0) { return compareArrayIndices(left, *map->mapValues(), rightIndices, flags); } return result;

duanmeng · 2023-09-12T07:59:55Z

@mbasmanova Hi Masha, I've fixed all the comments, refactored the UT codes, and improved the coverage according to your guidance. Could you help to take a look?

mbasmanova

@duanmeng Thank you for iterating on this PR. This is shaping up nicely. Some comments below.

mbasmanova · 2023-09-12T08:26:05Z

velox/exec/ContainerRowSerde.cpp

+
+    if (leftNull || rightNull) {
+      auto result = BaseVector::compareNulls(leftNull, rightNull, flags);
+      if (result && *result == 0) {


I find this a bit hard to read. Consider being more explicit:

if (result.has_value() && result.value() == 0)

mbasmanova · 2023-09-12T08:26:30Z

velox/exec/ContainerRowSerde.cpp

    auto result = compareSwitch(left, *child, wrappedIndex, flags);
-    if (result) {
-      return result;
+    if (result && *result == 0) {


mbasmanova · 2023-09-12T08:34:18Z

velox/exec/ContainerRowSerde.h

@@ -44,6 +44,15 @@ class ContainerRowSerde {
      const Type* type,
      CompareFlags flags);

+  /// Return std::nullopt if encountered in NullHandlingMode::StopAtNull mode.


I find this comment a bit cryptic. Maybe, use the comment from BaseVector::compare as a base.

/// Returns < 0 if 'left' is less than 'right' at 'index', 0 if /// equal and > 0 otherwise. If flags.nullHandlingMode is StopAtNull, /// returns std::nullopt if either 'left' or 'right' value is null or contains a null.

Also, let's document existing 'compare' methods to clarify that these support only NoStop for flags.nullHandlingMode, consider NULL equal to NULL.

mbasmanova · 2023-09-12T08:39:53Z

velox/exec/tests/ContainerRowSerdeTest.cpp

@@ -35,6 +35,39 @@ class ContainerRowSerdeTest : public testing::Test,
    return position;
  }

+  std::vector<std::shared_ptr<ByteStream>> serialize(


This method can be simplified to just serialize the data and return a list of positions. No need to create and return ByteStreams. These can be created on the fly from the positions in testCompareWithNulls. I think this would also remove the need for rewindByteStreams method.

Thanks for your suggestion, It is much cleaner now.

mbasmanova · 2023-09-12T08:40:33Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+  void testCompareWithNulls(
+      const DecodedVector& decodedVector,
+      const std::vector<HashStringAllocator::Position>& positions,
+      const std::vector<std::shared_ptr<ByteStream>>& byteStreams,


Per comment above, let's remove byteStreams parameter and create ByteStreeams from positions on the fly.

mbasmanova · 2023-09-12T08:42:00Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+          expected.at(i),
+          ContainerRowSerde::compareWithNulls(
+              *byteStreams.at(i), decodedVector, i, compareFlags)
+              .value());


This assumes that compareWithNulls cannot return std::nullopt? Is this a valid assumption? Don't we want to allow testing cases where compareWithNulls returns std::nullopt?

mbasmanova · 2023-09-12T08:42:43Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+    }
+
+    auto res = ContainerRowSerde::compareWithNulls(
+        *byteStreams.at(3), decodedVector, 3, compareFlags);


Hard-coding index '3' here makes this method hard to use. Let's change expected to std::vector<std::optional<int32_t>> and remove expectedOptionVal.

mbasmanova · 2023-09-12T08:44:18Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+      byteStreams,
+      {0, 1, -1},
+      std::nullopt,
+      compareFlags);


Since we only test different values of equalsOnly and nullHandlingMode, the test may be more readable if we change testCompareWithNulls to take these 2 values instead of CompareFlags.

mbasmanova

@duanmeng Look great % a few remaining nits.

mbasmanova · 2023-09-12T11:35:30Z

velox/exec/ContainerRowSerde.h

+  /// Returns < 0 if 'left' is less than 'right' at 'index', 0 if
+  /// equal and > 0 otherwise. If flags.nullHandlingMode is StopAtNull,
+  /// returns std::nullopt if either 'left' or 'right' value is null or contains
+  /// a null. If flags.nullHandlingMode is NoStop, returns


If flags.nullHandlingMode is NoStop, returns
std::optional<int32_t>(0) if both sides are nulls

Let's remove this sentence. It is missing in comments for other versions of this method.

Alternatively, let's say that "If flags.nullHandlingMode is NoStop then NULL is considered equal to NULL."

velox/exec/tests/ContainerRowSerdeTest.cpp

mbasmanova · 2023-09-12T11:36:46Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+      ContainerRowSerde::serialize(*data, i, out);
+      allocator_.finishWrite(out, 0);
+
+      auto cur = std::make_shared<ByteStream>();


The logic using 'cur' is not needed. Let's remove.

mbasmanova · 2023-09-12T11:37:16Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+        mode};
+
+    for (auto i = 0; i < expected.size(); ++i) {
+      ByteStream buf;


buf -> stream

mbasmanova · 2023-09-12T11:37:37Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+      {1, 2, 3, 4},
+  });
+
+  std::vector<HashStringAllocator::Position> positions =


velox/exec/tests/ContainerRowSerdeTest.cpp

mbasmanova · 2023-09-12T11:39:18Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+      serializeWithPositions(data);
+
+  auto someNulls = makeNullableFlatVector<int32_t>({
+      {1},


not need for {} around numbers

mbasmanova · 2023-09-12T11:39:39Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+  auto rowVector = makeRowVector({someNulls});
+  SelectivityVector rows(rowVector->size());
+  DecodedVector decodedVector;
+  decodedVector.decode(*rowVector, rows);


You can remove 'rows' parameter and then remove 'rows' variable.

DecodedVector decodedVector(*decodedVector);

mbasmanova · 2023-09-12T12:08:39Z

velox/exec/tests/ContainerRowSerdeTest.cpp

+      {1, 2, std::nullopt, 4},
+      {1, 5},
+  });
+  DecodedVector decodedVector;


You can combine these 2 lines into one:

DecodedVector decodedVector(*arrayVector);

Ditto other places.

mbasmanova

Thanks.

facebook-github-bot · 2023-09-12T12:14:16Z

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

facebook-github-bot · 2023-09-12T12:28:00Z

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

facebook-github-bot · 2023-09-12T17:14:23Z

@mbasmanova merged this pull request in 34dd00c.

conbench-facebook · 2023-09-12T17:49:41Z

Conbench analyzed the 1 benchmark run on commit 34dd00c3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: In Presto, min and max check for null array elements, map values, and struct fields and throw, but in Velox, they don't. For example, the following SQL should throw an exception, "**ARRAY comparison not supported for arrays with null elements**". ```SQL SELECT min(col0) FROM (VALUES (array [1, 2, 3, 4]), (array [null, 1])) AS t(col0) ``` This PR adds a comparison method in `SingleValueAccumulator` using the method added in #6419 to make `min/max` aggregate functions that compare values of complex types able to detect cases when complex type values contain nulls, making the following guarantees, - ARRAY comparison is not supported for arrays with null elements - ROW comparison is not supported for fields with null elements - MAP comparison is not supported for null value elements Part of #6314 Pull Request resolved: #6570 Reviewed By: xiaoxmeng Differential Revision: D49315892 Pulled By: mbasmanova fbshipit-source-id: dffb780ac6252c5e85fb3700db452cf2d7ace8df

Summary: Presto aggregate functions that compare values of complex types need to detect cases when complex type values contain nulls. The new ContainerRowSerde::compareWithNulls API allows to signal the case when complex type value contains a null. Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6419 Reviewed By: xiaoxmeng Differential Revision: D49187047 Pulled By: mbasmanova fbshipit-source-id: 326c6b5e47cd1050ec456b41e7ff56ab3c75c3a6

…okincubator#6570) Summary: In Presto, min and max check for null array elements, map values, and struct fields and throw, but in Velox, they don't. For example, the following SQL should throw an exception, "**ARRAY comparison not supported for arrays with null elements**". ```SQL SELECT min(col0) FROM (VALUES (array [1, 2, 3, 4]), (array [null, 1])) AS t(col0) ``` This PR adds a comparison method in `SingleValueAccumulator` using the method added in facebookincubator#6419 to make `min/max` aggregate functions that compare values of complex types able to detect cases when complex type values contain nulls, making the following guarantees, - ARRAY comparison is not supported for arrays with null elements - ROW comparison is not supported for fields with null elements - MAP comparison is not supported for null value elements Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6570 Reviewed By: xiaoxmeng Differential Revision: D49315892 Pulled By: mbasmanova fbshipit-source-id: dffb780ac6252c5e85fb3700db452cf2d7ace8df

Summary: Presto aggregate functions that compare values of complex types need to detect cases when complex type values contain nulls. The new ContainerRowSerde::compareWithNulls API allows to signal the case when complex type value contains a null. Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6419 Reviewed By: xiaoxmeng Differential Revision: D49187047 Pulled By: mbasmanova fbshipit-source-id: 326c6b5e47cd1050ec456b41e7ff56ab3c75c3a6

…okincubator#6570) Summary: In Presto, min and max check for null array elements, map values, and struct fields and throw, but in Velox, they don't. For example, the following SQL should throw an exception, "**ARRAY comparison not supported for arrays with null elements**". ```SQL SELECT min(col0) FROM (VALUES (array [1, 2, 3, 4]), (array [null, 1])) AS t(col0) ``` This PR adds a comparison method in `SingleValueAccumulator` using the method added in facebookincubator#6419 to make `min/max` aggregate functions that compare values of complex types able to detect cases when complex type values contain nulls, making the following guarantees, - ARRAY comparison is not supported for arrays with null elements - ROW comparison is not supported for fields with null elements - MAP comparison is not supported for null value elements Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6570 Reviewed By: xiaoxmeng Differential Revision: D49315892 Pulled By: mbasmanova fbshipit-source-id: dffb780ac6252c5e85fb3700db452cf2d7ace8df

Summary: Presto aggregate functions that compare values of complex types need to detect cases when complex type values contain nulls. The new ContainerRowSerde::compareWithNulls API allows to signal the case when complex type value contains a null. Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6419 Reviewed By: xiaoxmeng Differential Revision: D49187047 Pulled By: mbasmanova fbshipit-source-id: 326c6b5e47cd1050ec456b41e7ff56ab3c75c3a6

…okincubator#6570) Summary: In Presto, min and max check for null array elements, map values, and struct fields and throw, but in Velox, they don't. For example, the following SQL should throw an exception, "**ARRAY comparison not supported for arrays with null elements**". ```SQL SELECT min(col0) FROM (VALUES (array [1, 2, 3, 4]), (array [null, 1])) AS t(col0) ``` This PR adds a comparison method in `SingleValueAccumulator` using the method added in facebookincubator#6419 to make `min/max` aggregate functions that compare values of complex types able to detect cases when complex type values contain nulls, making the following guarantees, - ARRAY comparison is not supported for arrays with null elements - ROW comparison is not supported for fields with null elements - MAP comparison is not supported for null value elements Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6570 Reviewed By: xiaoxmeng Differential Revision: D49315892 Pulled By: mbasmanova fbshipit-source-id: dffb780ac6252c5e85fb3700db452cf2d7ace8df

Summary: Presto aggregate functions that compare values of complex types need to detect cases when complex type values contain nulls. The new ContainerRowSerde::compareWithNulls API allows to signal the case when complex type value contains a null. Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6419 Reviewed By: xiaoxmeng Differential Revision: D49187047 Pulled By: mbasmanova fbshipit-source-id: 326c6b5e47cd1050ec456b41e7ff56ab3c75c3a6

…okincubator#6570) Summary: In Presto, min and max check for null array elements, map values, and struct fields and throw, but in Velox, they don't. For example, the following SQL should throw an exception, "**ARRAY comparison not supported for arrays with null elements**". ```SQL SELECT min(col0) FROM (VALUES (array [1, 2, 3, 4]), (array [null, 1])) AS t(col0) ``` This PR adds a comparison method in `SingleValueAccumulator` using the method added in facebookincubator#6419 to make `min/max` aggregate functions that compare values of complex types able to detect cases when complex type values contain nulls, making the following guarantees, - ARRAY comparison is not supported for arrays with null elements - ROW comparison is not supported for fields with null elements - MAP comparison is not supported for null value elements Part of facebookincubator#6314 Pull Request resolved: facebookincubator#6570 Reviewed By: xiaoxmeng Differential Revision: D49315892 Pulled By: mbasmanova fbshipit-source-id: dffb780ac6252c5e85fb3700db452cf2d7ace8df

…7138) Summary: Presto aggregate functions that compare values of complex types must detect cases when complex values contain nulls. The new ContainerRowSerde::compareWithNulls API allows signaling the case when a complex type value contains a null when comparing ByteStream with ByteStream. Similar to #6419. Pull Request resolved: #7138 Reviewed By: Yuhta Differential Revision: D50593886 Pulled By: mbasmanova fbshipit-source-id: b0946ab7b1539b1656c831680be0df314f9243af

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

mbasmanova requested a review from laithsakka September 5, 2023 13:54

duanmeng force-pushed the cmpnull branch 3 times, most recently from 1d608db to 21b8a91 Compare September 11, 2023 09:24

Add compareWithNulls for row/map/array in ContainerRowSerde

1f4041e

duanmeng force-pushed the cmpnull branch from 21b8a91 to 1f4041e Compare September 11, 2023 09:38

duanmeng changed the title ~~[WIP]Add compareWithNulls for row/map/array in ContainerRowSerde~~ Add compareWithNulls for row/map/array in ContainerRowSerde Sep 11, 2023

duanmeng mentioned this pull request Sep 11, 2023

min and max aggregate functions should reject complex type inputs with nulls #6314

Open

mbasmanova reviewed Sep 11, 2023

View reviewed changes

duanmeng force-pushed the cmpnull branch 3 times, most recently from e12a786 to 4564074 Compare September 11, 2023 16:09

resolve comments

364104b

kagamiori reviewed Sep 11, 2023

View reviewed changes

duanmeng force-pushed the cmpnull branch from 4564074 to bd42edd Compare September 11, 2023 17:21

add map ut

b05c340

duanmeng force-pushed the cmpnull branch from bd42edd to b05c340 Compare September 12, 2023 06:54

refactor ut

6af8fa5

mbasmanova reviewed Sep 12, 2023

View reviewed changes

resolve comments

932642a

duanmeng force-pushed the cmpnull branch from 2bbb279 to 932642a Compare September 12, 2023 11:17

mbasmanova reviewed Sep 12, 2023

View reviewed changes

mbasmanova changed the title ~~Add compareWithNulls for row/map/array in ContainerRowSerde~~ Add ContainerRowSerde::compareWithNulls API Sep 12, 2023

mbasmanova reviewed Sep 12, 2023

View reviewed changes

mbasmanova approved these changes Sep 12, 2023

View reviewed changes

resolve comments

952a601

duanmeng force-pushed the cmpnull branch from cfe1b35 to 952a601 Compare September 12, 2023 12:23

facebook-github-bot closed this in 34dd00c Sep 12, 2023

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

duanmeng mentioned this pull request Sep 15, 2023

Fix handling of nested nulls in min and max Presto aggregates #6570

Closed

duanmeng deleted the cmpnull branch October 11, 2023 15:46

duanmeng mentioned this pull request Oct 19, 2023

Add ContainerRowSerde::compareWithNulls API for ByteStream compare #7138

Closed

Add ContainerRowSerde::compareWithNulls API #6419

Add ContainerRowSerde::compareWithNulls API #6419

Conversation

duanmeng commented Sep 5, 2023 • edited by mbasmanova

netlify bot commented Sep 5, 2023 • edited

✅ Deploy Preview for meta-velox canceled.

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

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

kagamiori 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

duanmeng commented Sep 12, 2023

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

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

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

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

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 12, 2023

facebook-github-bot commented Sep 12, 2023

facebook-github-bot commented Sep 12, 2023

conbench-facebook bot commented Sep 12, 2023

duanmeng commented Sep 5, 2023 •

edited by mbasmanova

netlify bot commented Sep 5, 2023 •

edited