Implement mergewith for bytesRange and bytesValue #297

atanu1991 · 2021-09-24T18:03:23Z

Implement BytesRange::mergeWith and BytesValues::mergeWith
This commit is a follow up of #119.

amaliujia · 2021-09-27T17:24:03Z

Per this PR, seems to me that I will need to address Merging ranges/byte values in MultiRange::mergeWith in #299?

velox/type/Filter.cpp

amaliujia · 2021-09-27T17:33:03Z

Per this PR, seems to me that I will need to address Merging ranges/byte values in MultiRange::mergeWith in #299?

cc @mbasmanova to confirm?

velox/type/tests/FilterBuilder.h

atanu1991 · 2021-10-01T01:41:58Z

Per this PR, seems to me that I will need to address Merging ranges/byte values in MultiRange::mergeWith in #299?

I have addressed that here. Probably there would some changes required to consider merging bytes ranges and values based of your implementation in #299

mbasmanova

@atanu1991 Overall looks good. Some comments below.

Consider adding BytesRange::toString() method to help debugging test failures:

  std::string toString() const final {
    return fmt::format(
        "BytesRange: {}{}, {}{} {}",
        (lowerUnbounded_ || lowerExclusive_) ? "(" : "[",
        lowerUnbounded_ ? "..." : lower_,
        upperUnbounded_ ? "..." : upper_,
        (upperUnbounded_ || upperExclusive_) ? ")" : "]",
        nullAllowed_ ? "with nulls" : "no nulls");
  }

mbasmanova · 2021-10-05T09:26:55Z

velox/type/Filter.cpp

+      std::string upper = "", lower = "";
+
+      // TODO:
+      // Handle the case of same filter being upperUnbounded and


Why is this a TODO? What makes it difficult to support this case?

velox/type/Filter.cpp

mbasmanova · 2021-10-05T10:11:12Z

velox/type/Filter.cpp

+  }
+}
+
+std::unique_ptr<Filter> MultiRange::mergeWith(const Filter* other) const {


Is this just a move? Are there any changes? It would be easier to review if this method stayed in place.

The function was in line 475. It has not only just been moved but also functionality was added. Line 1060 to 1116 was added.

I can move the whole thing back to 475 line, but I feel its right to have the Multirange::mergewith at the end of the file

It would be easier to review the changes if the function was moved back. Otherwise, I can't tell what has changed.

velox/type/Filter.cpp

velox/type/tests/FilterTest.cpp

mbasmanova · 2021-10-05T10:22:47Z

There are some typos in the PR title and description.

Title:

Implement mergewith for bytesRange and bytesValue

Implement BytesRange::mergeWith and BytesValues::mergeWith

Description:

"Implement mergewith for bytesRange and bytesValue" is redundant as it repeats the title. Let's remove.
"and introduces mergeWith implmentation for BytesRange and BytesValues." - this is also redundant. It is just a different way to say what the title already says.

(New PR) Handling case of merging multiple byte ranges which results in MultiRange as the output

Any particular reason not to include this functionality in this PR? Would this require significant refactoring of the current implementation?

atanu1991 · 2021-10-06T09:37:44Z

Thanks a lot for the detailed review.

@mbasmanova The case where the same range is both lower and upper unbounded might result in a MultiRange when merged with another range. This makes it a bit different from the existing cases now, and needs to be handled in a special way. Hence I thought handling it in a new PR would be better. Let me know your thoughts.

mbasmanova · 2021-10-06T09:39:13Z

The case where the same range is both lower and upper unbounded might result in a MultiRange when merged with another range.

Can you give an example? How are you planning to handle this case?

mbasmanova · 2021-10-06T09:41:01Z

the same range is both lower and upper unbounded

Is this possible? That's just kAlwaysTrue. We can add a check to the constructor to make sure if such range is never created.

mbasmanova · 2021-10-06T09:43:14Z

I see that comments in Filter.h are not explaining that MultiRange filter is expected to be a combination of non-overlapping filters and in general each filter must be restrictive, e.g. cannot pass all non-null values unless it is kAlwaysTrue or kIsNotNull. We need to clarify that.

atanu1991 · 2021-10-06T09:44:22Z

The case where the same range is both lower and upper unbounded might result in a MultiRange when merged with another range.

Can you give an example? How are you planning to handle this case?

Example a range like (<=c, >=q) ("q", true, false "c", true, false) merged with [a-z]
The output of this would be [a-c] and [q-z] right?

I havent quite thought of exactly how to handle this right now.
Was planning to have a special if block " if (upperunbounded && lowerunbounded) " and handle it there.

atanu1991 · 2021-10-06T09:45:22Z

the same range is both lower and upper unbounded

Is this possible? That's just kAlwaysTrue. We can add a check to the constructor to make sure if such range is never created.

If we can have a such a check and this case is not required then there would be no follow ups

mbasmanova · 2021-10-06T09:48:15Z

Example a range like (<=c, >=q) ("q", true, false "c", true, false)

That would be a MultiRange, right? Not kBytesRange. The TODO is currently in the code that merges two BytesRange's. Is it in the right place?

velox/type/Filter.cpp

atanu1991 · 2021-10-06T09:56:02Z

Example a range like (<=c, >=q) ("q", true, false "c", true, false)

That would be a MultiRange, right? Not kBytesRange. The TODO is currently in the code that merges two BytesRange's. Is it in the right place?

I see. So basically is it impossible to create a BytesRange object with ("q", true, false "c", true, false)
If yes, then should we have that assert in the BytesRange Constructor?

And yes if this is a MultiRange then there is no TODO remaining for this PR

mbasmanova · 2021-10-06T10:01:35Z

BytesRange can only represent a single range. It cannot represent <= a OR >= b. Let's add a check to the constructor in a separate PR.

velox/type/Filter.cpp

mbasmanova · 2021-10-06T10:23:34Z

velox/type/Filter.cpp

-      bool bothNanAllowed = nanAllowed_ && multiRangeOther->nanAllowed_;
+      bool bothNanAllowed = nanAllowed_;
+
+      std::vector<const Filter*> otherFilters;
      std::vector<std::unique_ptr<Filter>> merged;


nit: Let's move these variables closer to where they are used.

mbasmanova

Looks good to me. Thank you for the contribution.

facebook-github-bot · 2021-10-06T18:44:57Z

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

) Summary: Implement BytesRange::mergeWith and BytesValues::mergeWith This commit is a follow up of facebookincubator#119. Pull Request resolved: facebookincubator#297 Reviewed By: mbasmanova Differential Revision: D31444325 Pulled By: atanu1991 fbshipit-source-id: 3e8d8bb2645455fcaf24ebd18d6c122613221958

facebook-github-bot · 2021-10-07T18:22:46Z

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

…adapt upstream Arrow (facebookincubator#297) * separate arrow version for gazelle and velox backend * separate velox and gazelle module to support different arrow version

atanu1991 force-pushed the filterbytes branch from bda4971 to 3621f6d Compare September 24, 2021 18:16

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 24, 2021

amaliujia reviewed Sep 27, 2021

View reviewed changes

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

atanu1991 force-pushed the filterbytes branch 2 times, most recently from 898655f to ce968d5 Compare October 1, 2021 00:15

atanu1991 changed the title ~~[WIP][Draft] Implement mergewith for bytesRange and bytesValue~~ Implement mergewith for bytesRange and bytesValue Oct 1, 2021

atanu1991 force-pushed the filterbytes branch from ce968d5 to a12933b Compare October 1, 2021 00:18

mbasmanova reviewed Oct 1, 2021

View reviewed changes

velox/type/tests/FilterBuilder.h Show resolved Hide resolved

atanu1991 force-pushed the filterbytes branch 6 times, most recently from 569d41c to 2a55e6c Compare October 5, 2021 04:06

mbasmanova reviewed Oct 5, 2021

View reviewed changes

atanu1991 force-pushed the filterbytes branch from 2a55e6c to e96aaef Compare October 6, 2021 09:24

atanu1991 force-pushed the filterbytes branch from e96aaef to dbd4dfc Compare October 6, 2021 09:52

atanu1991 commented Oct 6, 2021

View reviewed changes

velox/type/Filter.cpp Show resolved Hide resolved

mbasmanova reviewed Oct 6, 2021

View reviewed changes

velox/type/Filter.cpp Show resolved Hide resolved

atanu1991 force-pushed the filterbytes branch from dbd4dfc to e937b58 Compare October 6, 2021 10:06

mbasmanova reviewed Oct 6, 2021

View reviewed changes

mbasmanova approved these changes Oct 6, 2021

View reviewed changes

atanu1991 force-pushed the filterbytes branch from e937b58 to d4e6626 Compare October 6, 2021 18:44

atanu1991 force-pushed the filterbytes branch from d4e6626 to 9ed6a2f Compare October 7, 2021 18:22

facebook-github-bot closed this in d6bfa3d Oct 7, 2021

rterror mentioned this pull request Jun 7, 2023

Implement Filter::mergeWith for BytesRange and BytesValue #153

Open

rui-mo added a commit to rui-mo/velox that referenced this pull request Jun 8, 2023

Use correct name in struct type (facebookincubator#297)

2d0dd93

liujiayi771 pushed a commit to liujiayi771/velox that referenced this pull request Jun 9, 2023

Use correct name in struct type (facebookincubator#297)

3213997

PHILO-HE pushed a commit to PHILO-HE/velox that referenced this pull request Jun 27, 2023

Use correct name in struct type (facebookincubator#297)

13c10c9

zhli1142015 pushed a commit to zhli1142015/velox that referenced this pull request Jul 3, 2023

Use correct name in struct type (facebookincubator#297)

0c7e0ad

Yohahaha pushed a commit to Yohahaha/velox that referenced this pull request Jul 4, 2023

Use correct name in struct type (facebookincubator#297)

8ae1d76

chenxu14 pushed a commit to chenxu14/velox that referenced this pull request Jul 5, 2023

Use correct name in struct type (facebookincubator#297)

b56b3ee

PHILO-HE pushed a commit to PHILO-HE/velox that referenced this pull request Jul 17, 2023

Use correct name in struct type (facebookincubator#297)

38312fa

rui-mo added a commit to rui-mo/velox that referenced this pull request Jul 21, 2023

Use correct name in struct type (facebookincubator#297)

5f60805

rui-mo added a commit to rui-mo/velox that referenced this pull request Jul 24, 2023

Use correct name in struct type (facebookincubator#297)

e110c25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mergewith for bytesRange and bytesValue #297

Implement mergewith for bytesRange and bytesValue #297

atanu1991 commented Sep 24, 2021 •

edited

Loading

amaliujia commented Sep 27, 2021

amaliujia commented Sep 27, 2021

atanu1991 commented Oct 1, 2021

mbasmanova left a comment

mbasmanova Oct 5, 2021

mbasmanova Oct 5, 2021

atanu1991 Oct 6, 2021 •

edited

Loading

mbasmanova Oct 6, 2021

mbasmanova commented Oct 5, 2021

atanu1991 commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

atanu1991 commented Oct 6, 2021

atanu1991 commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

atanu1991 commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

mbasmanova Oct 6, 2021

mbasmanova left a comment

facebook-github-bot commented Oct 6, 2021

facebook-github-bot commented Oct 7, 2021

Implement mergewith for bytesRange and bytesValue #297

Implement mergewith for bytesRange and bytesValue #297

Conversation

atanu1991 commented Sep 24, 2021 • edited Loading

amaliujia commented Sep 27, 2021

amaliujia commented Sep 27, 2021

atanu1991 commented Oct 1, 2021

mbasmanova left a comment

Choose a reason for hiding this comment

mbasmanova Oct 5, 2021

Choose a reason for hiding this comment

mbasmanova Oct 5, 2021

Choose a reason for hiding this comment

atanu1991 Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

mbasmanova Oct 6, 2021

Choose a reason for hiding this comment

mbasmanova commented Oct 5, 2021

atanu1991 commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

atanu1991 commented Oct 6, 2021

atanu1991 commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

atanu1991 commented Oct 6, 2021

mbasmanova commented Oct 6, 2021

mbasmanova Oct 6, 2021

Choose a reason for hiding this comment

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 6, 2021

facebook-github-bot commented Oct 7, 2021

atanu1991 commented Sep 24, 2021 •

edited

Loading

atanu1991 Oct 6, 2021 •

edited

Loading