Add check for overlapping ranges of ARRAY and MAP vectors #10960

Yuhta · 2024-09-10T15:48:27Z

Summary: We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that, and also add the check to BaseVector::validate() method to make it clear it is not valid. Later we will also add the check to ArrayVectorBase constructor.

Differential Revision: D62212238

netlify · 2024-09-10T15:48:45Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`ad6d14d`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66e8a5c943bb830008d2f712

facebook-github-bot · 2024-09-10T15:48:48Z

This pull request was exported from Phabricator. Differential Revision: D62212238

xiaoxmeng

@Yuhta thanks for the change % minors.

xiaoxmeng · 2024-09-12T22:47:51Z

velox/docs/develop/vectors.rst

-contains offsets and sizes buffers and an elements vector. Offsets and sizes
-are 32-bit integers.
+contains offsets and sizes buffers and an elements vector. Offsets and sizes are
+32-bit integers. The non-null non-empty ranges formed by offsets and sizes in a


Does offsets need to in order in elements vector? Thanks!

No it does not. But by default this is the case for most vectors, so we leverage that in the fast path.

velox/vector/ComplexVector.h

xiaoxmeng · 2024-09-12T23:07:36Z

velox/vector/ComplexVector.cpp

+template <bool kHasNulls>
+vector_size_t ArrayVectorBase::nextNonEmpty(vector_size_t i) const {
+  while (i < size() &&
+         ((kHasNulls && bits::isBitNull(rawNulls(), i)) || rawSizes_[i] <= 0)) {


rawSizes_ could be < 0? Thanks!

It should not, but some application may generate such a thing as intermediate state. This is checked in ArrayVectorBase::validateArrayVectorBase before calling this check.

xiaoxmeng · 2024-09-12T23:08:49Z

velox/vector/ComplexVector.cpp

+         ((kHasNulls && bits::isBitNull(rawNulls(), i)) || rawSizes_[i] <= 0)) {
+    ++i;
+  }
+  return i;


Shall we return std::optional if not found?

Probably no need, we will need to check i < size() once again anyway and this won't save us anything in terms of efficiency (it's probably even one more registry used) or code readability.

velox/vector/ComplexVector.cpp

xiaoxmeng · 2024-09-12T23:14:55Z

velox/vector/ComplexVector.cpp

+  i = nextNonEmpty<kHasNulls>(i);
+  if (i >= size()) {
+    return false;
+  }


How about

vector_size_t prev{-1}; for (;;) { const auto next = ...; ... if (prev != -1 && ...) { return false; } prev = next; }

Updated with curr and next. Feels a little bit weird since these are usually used for pointers.

Can you merge the code in l824 into the loop? curr{-1} can indicate the initial case? Thanks!

There is call to rawOffsets_[curr] so curr = -1 is not valid here

velox/vector/ComplexVector.cpp

xiaoxmeng · 2024-09-12T23:22:27Z

velox/vector/ComplexVector.cpp

+      indices.push_back(i);
+    }
+  }
+  std::sort(indices.begin(), indices.end(), [&](auto i, auto j) {


rawSizes_ could be < 0? If not, why maybeHaveOverlappingRanges check is not sufficient? Thanks!

rawOffsets_ could be out of order

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that, and also add the check to `BaseVector::validate()` method to make it clear it is not valid. Later we will also add the check to `ArrayVectorBase` constructor. Differential Revision: D62212238

facebook-github-bot · 2024-09-16T14:35:34Z

This pull request was exported from Phabricator. Differential Revision: D62212238

facebook-github-bot · 2024-09-16T14:41:06Z

This pull request was exported from Phabricator. Differential Revision: D62212238

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that, and also add the check to `BaseVector::validate()` method to make it clear it is not valid. Later we will also add the check to `ArrayVectorBase` constructor. Differential Revision: D62212238

facebook-github-bot · 2024-09-16T15:24:56Z

This pull request was exported from Phabricator. Differential Revision: D62212238

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that. In the future we will also add the check to `BaseVector::validate()` and `ArrayVectorBase` constructor to make it clear it is not allowed. Differential Revision: D62212238

facebook-github-bot · 2024-09-16T15:30:44Z

This pull request was exported from Phabricator. Differential Revision: D62212238

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that. In the future we will also add the check to `BaseVector::validate()` and `ArrayVectorBase` constructor to make it clear it is not allowed. Differential Revision: D62212238

xiaoxmeng · 2024-09-16T21:04:52Z

velox/vector/ComplexVector.h

+  template <bool kHasNulls>
+  bool maybeHaveOverlappingRanges() const;
+
+  // Return the next non-null non-empty array/map after `index'.


s/Return/Returns/

xiaoxmeng · 2024-09-16T21:07:11Z

velox/vector/ComplexVector.cpp

+  i = nextNonEmpty<kHasNulls>(i);
+  if (i >= size()) {
+    return false;
+  }


Can you merge the code in l824 into the loop? curr{-1} can indicate the initial case? Thanks!

xiaoxmeng

@Yuhta LGTM. Thanks!

facebook-github-bot · 2024-09-16T21:23:07Z

This pull request was exported from Phabricator. Differential Revision: D62212238

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that. In the future we will also add the check to `BaseVector::validate()` and `ArrayVectorBase` constructor to make it clear it is not allowed. Reviewed By: xiaoxmeng Differential Revision: D62212238

facebook-github-bot · 2024-09-16T21:27:09Z

This pull request was exported from Phabricator. Differential Revision: D62212238

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that. In the future we will also add the check to `BaseVector::validate()` and `ArrayVectorBase` constructor to make it clear it is not allowed. Reviewed By: xiaoxmeng Differential Revision: D62212238

facebook-github-bot · 2024-09-16T21:31:35Z

This pull request was exported from Phabricator. Differential Revision: D62212238

facebook-github-bot · 2024-09-16T21:35:59Z

This pull request was exported from Phabricator. Differential Revision: D62212238

…cubator#10960) Summary: Pull Request resolved: facebookincubator#10960 We don't allow overlapping ranges in ARRAY and MAP vectors. However this is not clear in the code, so we add a method for user to check that. In the future we will also add the check to `BaseVector::validate()` and `ArrayVectorBase` constructor to make it clear it is not allowed. Reviewed By: xiaoxmeng Differential Revision: D62212238

facebook-github-bot · 2024-09-16T21:40:21Z

This pull request was exported from Phabricator. Differential Revision: D62212238

facebook-github-bot · 2024-09-17T07:01:09Z

This pull request has been merged in 0a9c481.

conbench-facebook · 2024-09-17T07:48:25Z

Conbench analyzed the 1 benchmark run on commit 0a9c4812.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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 10, 2024

facebook-github-bot added the fb-exported label Sep 10, 2024

Yuhta changed the title ~~Add check to forbid overlap ranges for ARRAY and MAP vectors~~ Add check for overlapping ranges of ARRAY and MAP vectors Sep 10, 2024

xiaoxmeng reviewed Sep 12, 2024

View reviewed changes

Yuhta force-pushed the export-D62212238 branch from 876919c to 851c3e1 Compare September 16, 2024 14:35

Yuhta force-pushed the export-D62212238 branch from 851c3e1 to 396b8ea Compare September 16, 2024 14:41

Yuhta force-pushed the export-D62212238 branch from 396b8ea to d3bcb9e Compare September 16, 2024 15:25

Yuhta force-pushed the export-D62212238 branch from d3bcb9e to 77e95dd Compare September 16, 2024 15:30

xiaoxmeng reviewed Sep 16, 2024

View reviewed changes

xiaoxmeng approved these changes Sep 16, 2024

View reviewed changes

Yuhta force-pushed the export-D62212238 branch from 77e95dd to 1387e1f Compare September 16, 2024 21:23

Yuhta force-pushed the export-D62212238 branch from 1387e1f to e021540 Compare September 16, 2024 21:27

Yuhta force-pushed the export-D62212238 branch from e021540 to 888c044 Compare September 16, 2024 21:31

Yuhta force-pushed the export-D62212238 branch from 888c044 to 78e74e8 Compare September 16, 2024 21:35

Yuhta force-pushed the export-D62212238 branch from 78e74e8 to ad6d14d Compare September 16, 2024 21:40

facebook-github-bot closed this in 0a9c481 Sep 17, 2024

facebook-github-bot added the Merged label Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for overlapping ranges of ARRAY and MAP vectors #10960

Add check for overlapping ranges of ARRAY and MAP vectors #10960

Yuhta commented Sep 10, 2024

netlify bot commented Sep 10, 2024 •

edited

Loading

facebook-github-bot commented Sep 10, 2024

xiaoxmeng left a comment

xiaoxmeng Sep 12, 2024

Yuhta Sep 16, 2024

xiaoxmeng Sep 12, 2024

Yuhta Sep 16, 2024 •

edited

Loading

xiaoxmeng Sep 12, 2024

Yuhta Sep 16, 2024 •

edited

Loading

xiaoxmeng Sep 12, 2024

Yuhta Sep 16, 2024

xiaoxmeng Sep 16, 2024

Yuhta Sep 16, 2024

xiaoxmeng Sep 12, 2024

Yuhta Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

xiaoxmeng Sep 16, 2024

xiaoxmeng Sep 16, 2024

xiaoxmeng left a comment

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 17, 2024

conbench-facebook bot commented Sep 17, 2024

Add check for overlapping ranges of ARRAY and MAP vectors #10960

Add check for overlapping ranges of ARRAY and MAP vectors #10960

Conversation

Yuhta commented Sep 10, 2024

netlify bot commented Sep 10, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Sep 10, 2024

xiaoxmeng 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

Yuhta Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuhta Sep 16, 2024 • edited Loading

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

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxmeng left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 16, 2024

facebook-github-bot commented Sep 17, 2024

conbench-facebook bot commented Sep 17, 2024

netlify bot commented Sep 10, 2024 •

edited

Loading

Yuhta Sep 16, 2024 •

edited

Loading

Yuhta Sep 16, 2024 •

edited

Loading