Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Enabling ascii encoding fast track based on row selection. #21

Closed
wants to merge 1 commit into from
Closed

[WIP] Enabling ascii encoding fast track based on row selection. #21

wants to merge 1 commit into from

Conversation

kgpai
Copy link
Contributor

@kgpai kgpai commented Aug 11, 2021

Notes:

  • The central idea here is we keep a mask (selectivity vector) for every SimpleVector which stores all ascii string information. For propagating asciness we simply intersect across the vectors.
  • All the string functions test pass (some tests which test for mostly ascii etc have been disabled)
  • Theres a bunch of code that is no longer relevant (like string encoding tracker etc, or the old getStringEncode functions etc ). I am going to remove them in subsequent commits.
  • Quite a few of the functions have to be renamed and they make sense in only old context.

@facebook-github-bot 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 Aug 11, 2021
@kgpai kgpai requested a review from mbasmanova August 11, 2021 09:13
mbasmanova
mbasmanova reviewed Aug 11, 2021
View reviewed changes
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgpai Thanks for working on this optimization. Some initial questions below.

velox/functions/common/StringFunctions.cpp
@@ -196,7 +196,7 @@ class SubstrFunction : public exec::VectorFunction {
BaseVector* stringsVector = args[0].get();
BaseVector* startsVector = args[1].get();
BaseVector* lengthsVector = noLengthVector ? nullptr : args[2].get();
auto stringArgStringEncoding = getStringEncodingOrUTF8(stringsVector);
auto stringArgStringEncoding = getStringEncodingOrUTF8(stringsVector, rows);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this method no longer take "rows"? The function runs only on "rows" and doesn't need to know whether the whole vector is ASCII, just whether these particular rows are ASCII.

velox/vector/SimpleVector.h

auto otherIter = SelectivityIterator(other);
vector_size_t idx;
while (otherIter.next(idx)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be more efficient way to combine two bitmaps. Would you take a look at SelectivityVector.h and BitUtil.h if there is an existing function? If not, let's make one.

velox/vector/SimpleVector.h

// If T is f4d::StringView, we create a bitmap to store asciiness for each
// string. A set bit means the corresponding string is ascii.
std::optional<std::shared_ptr<SelectivityVector>> asciiMap_ = std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason not to use empty SelectivityVector as the default?

kgpai
kgpai commented Aug 11, 2021
View reviewed changes
velox/functions/lib/StringEncodingUtils.h
} else {
return simpleVector->getStringEncoding().value();

if (simpleVector->hasStringAsciiMap()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove..

kgpai
kgpai commented Aug 11, 2021
View reviewed changes
velox/vector/SimpleVector.h
@@ -236,6 +236,10 @@ class SimpleVector : public BaseVector {
copyStringEncodingFrom(const BaseVector* vector) {
encodingMode_ =
vector->asUnchecked<SimpleVector<StringView>>()->getStringEncoding();
auto source = vector->asUnchecked<SimpleVector<StringView>>();
if (source->template hasStringAsciiMap()) {
asciiMap_ = std::optional(std::move(source->asciiMap_.value()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy..

@pedroerp pedroerp mentioned this pull request Aug 18, 2021
Closed
@kgpai kgpai closed this Sep 1, 2021
zhouyuan pushed a commit to zhouyuan/velox that referenced this pull request Jun 19, 2022
@JkSelf
add dwrf support in substrait (facebookincubator#21)
c37db15
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Jun 24, 2022
@JkSelf
add dwrf support in substrait (facebookincubator#21)
787982d
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Jun 30, 2022
@JkSelf @zhejiangxiaomai
add dwrf support in substrait (facebookincubator#21)
43d2d30
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Jul 20, 2022
@JkSelf @zhejiangxiaomai
add dwrf support in substrait (facebookincubator#21)
c352686
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Aug 2, 2022
@JkSelf @zhejiangxiaomai
add dwrf support in substrait (facebookincubator#21)
f65b6de
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Aug 12, 2022
@JkSelf @zhejiangxiaomai
add dwrf support in substrait (facebookincubator#21)
af5149a
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Aug 22, 2022
@JkSelf @zhejiangxiaomai
add dwrf support in substrait (facebookincubator#21)
806a2c3
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Sep 7, 2022
@JkSelf @zhejiangxiaomai
add dwrf support in substrait (facebookincubator#21)
27faf2a
This was referenced Oct 8, 2022
Closed
Closed
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Mar 27, 2023
@jinchengchenghh @JkSelf
fix make_decimal in long decimal (facebookincubator#21)
fb2f8f2
@billyean billyean mentioned this pull request Jun 7, 2023
Closed
@mayunlei mayunlei mentioned this pull request Jun 7, 2023
Open
@heary-cloud heary-cloud mentioned this pull request Dec 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbasmanova mbasmanova mbasmanova left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kgpai @mbasmanova @facebook-github-bot