Support for ascii encoding for a subset of rows. #32

kgpai · 2021-08-12T05:39:20Z

Currently for any String vector we store state on whether its fully ascii or not. When a vector is fully ascii it allows us to use the ascii fast path for any string computation. Most datasets are predominantly ascii , however presence of even one utf string will cause it to take utf path. We now maintain asciiness for the whole Vector but also maintain state on the rows we have processed.

Note:

We now also store asciiStateMap which is a selectivity vector of all rows we have processed so far; if we get a Selectivity vector for a row that we havent seen then we return nullopt else we return asciiness of whole vector.
We dont store the encoding mode state in the vector anymore.

TODO: This PR doesnt address case when a Vector is reused ; It might be possible then to have old / invalid ascii state. This will be addressed in a follow up PR.

mbasmanova

@kgpai Some comments and questions.

velox/expression/Expr.cpp

velox/functions/lib/StringEncodingUtils.h

velox/vector/ConstantVector.h

velox/expression/Expr.cpp

velox/vector/SimpleVector.h

mbasmanova

@kgpai Some comments and questions below.

velox/vector/SimpleVector.h

mbasmanova · 2021-08-17T14:29:39Z

velox/vector/SimpleVector.h

-  // If T is velox::StringView, specifies the string encoding mode
-  folly::Optional<functions::stringCore::StringEncodingMode> encodingMode_ =
-      folly::none;
+  // If T is f4d::StringView, we create a bitmap to store asciiness for each


f4d -> velox or better yet just drop it; "we create a bitmap to" can also be dropped for brevity: "If T is a StringView, stores ascii-ness ...."

velox/vector/SimpleVector.h

velox/functions/common/StringFunctions.cpp

velox/functions/common/tests/StringFunctionsTest.cpp

velox/vector/ConstantVector.h

velox/vector/SimpleVector.h

velox/expression/Expr.cpp

kgpai · 2021-08-18T18:14:09Z

In a succeeding PR , I will :

Remove StringEncoding enum
Add Tests and support for switch form.

velox/expression/ControlExpr.cpp

kgpai · 2021-08-19T06:43:26Z

velox/functions/common/tests/StringFunctionsTest.cpp

  // always then path
-  test("if(1=1, lower(C1), lower(C2))", StringEncodingMode::ASCII);


Some of these tests are no longer relevant (for e.g the test that checks encoding on partially populated vector. The other tests, test switch expressions which is coming in a subsequent pr.

mbasmanova

@kgpai A few quick questions.

velox/vector/SimpleVector.h

mbasmanova · 2021-08-19T19:04:48Z

velox/vector/SimpleVector.h

+
+      rows.template applyToSelected([&](auto row) {
+        asciiRows_.setValid(row, valid);
+        asciiSetRows_.setValid(row, true);


can this be shortened to asciiSetRows_.select(rows) ?

velox/vector/SimpleVector.h

mbasmanova · 2021-08-19T19:06:17Z

velox/vector/SimpleVector.h

+  copyAsciiDataFrom(const BaseVector* vector, const SelectivityVector& rows) {
+    auto source = vector->asUnchecked<SimpleVector<StringView>>();
+    if (vector->isConstantEncoding()) {
+      resizeAsciiRows(asciiRows_, rows.end());


resize calls should be the same in both branches, hence, can be moved outside of the "if"

velox/vector/SimpleVector.h

velox/expression/Expr.cpp

mbasmanova · 2021-08-19T19:58:27Z

velox/expression/Expr.cpp

-          maxEncoding(resultEncoding, inputVector->getStringEncoding().value());
-    }
+
+    resultVector->updateIsAscii(*inputVector, rows);


@kgpai My reading of the implementation of the updateIsAscii method is that it copies isAscii flags from other vector for the specified rows. However, here we need a different behavior. We want ascii flag to be true only for rows where all input rows are ascii.

Nice catch ! Fixed !

velox/expression/Expr.cpp

facebook-github-bot · 2021-08-20T20:47:38Z

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

facebook-github-bot · 2021-08-20T22:14:28Z

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

facebook-github-bot · 2021-08-20T22:33:12Z

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

mbasmanova

@kgpai Krishna, thank you for updating the PR. Here are some further comments on the non-test changes.

velox/expression/ControlExpr.cpp

velox/expression/Expr.cpp

velox/vector/SimpleVector.h

velox/expression/Expr.cpp

velox/expression/ControlExpr.cpp

velox/expression/Expr.cpp

velox/expression/ControlExpr.cpp

kgpai · 2021-08-21T08:31:15Z

velox/expression/Expr.cpp

-    (*result)->as<SimpleVector<StringView>>()->setStringEncoding(
-        *resultEncoding);
-  }
+  propagateIsAscii(vectorFunction_.get(), inputValues_, result, applyRows);


scanVectorFunctionInputsStringEncoding does calculate asciiness before calling applyVectorFunction. And we set it after the call via propagateIsAscii..

velox/expression/Expr.cpp

velox/vector/SimpleVector.h

kgpai · 2021-08-21T23:06:28Z

velox/vector/SelectivityVector.h

@@ -56,13 +56,15 @@ class SelectivityVector {
  // are set to false.
  static SelectivityVector empty(vector_size_t size);

-  void resize(int32_t size) {
+  void resize(int32_t size, bool value = true) {


I will create a seperate PR for this then.

facebook-github-bot · 2021-08-23T16:08:53Z

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

facebook-github-bot · 2021-08-23T16:40:24Z

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

mbasmanova

@kgpai Some further comments.

velox/vector/ConstantVector.h

velox/vector/SimpleVector.h

facebook-github-bot · 2021-09-08T20:05:19Z

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

facebook-github-bot · 2021-09-08T21:44:45Z

@kgpai merged this pull request in b48cd5f.

Summary: With the recent ascii compute changes (See: #32), the StringEncoding enum is redundant and no longer required. Pull Request resolved: #183 Reviewed By: mbasmanova Differential Revision: D30847517 Pulled By: kgpai fbshipit-source-id: 7c850f2536d17af179d343138b154ba934fb9c28

…incubator#27) * Filter validation for Parquet reader at runtime * Style * Style * Format Removed special handling for avg (facebookincubator#31) [OPPRO-173] Make batch size configurable (facebookincubator#32) support dwrf format

…bookincubator#32)

…incubator#27) * Filter validation for Parquet reader at runtime * Style * Style * Format Removed special handling for avg (facebookincubator#31) [OPPRO-173] Make batch size configurable (facebookincubator#32) support dwrf format

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

kgpai requested review from mbasmanova and pedroerp August 12, 2021 05:39

mbasmanova reviewed Aug 12, 2021

View reviewed changes

mbasmanova reviewed Aug 17, 2021

View reviewed changes

kgpai commented Aug 18, 2021

View reviewed changes

velox/expression/Expr.cpp Outdated Show resolved Hide resolved

kgpai commented Aug 19, 2021

View reviewed changes

kgpai force-pushed the fast_ascii branch from 8a7ca65 to e03ff58 Compare August 19, 2021 18:05

mbasmanova reviewed Aug 19, 2021

View reviewed changes

velox/expression/Expr.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Aug 19, 2021

View reviewed changes

velox/expression/Expr.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Aug 21, 2021

View reviewed changes

kgpai commented Aug 21, 2021

View reviewed changes

mbasmanova reviewed Aug 24, 2021

View reviewed changes

kgpai force-pushed the fast_ascii branch 8 times, most recently from 233f771 to 94f3792 Compare September 1, 2021 05:45

Fix after merge

52ec89a

kgpai force-pushed the fast_ascii branch from d66f0f7 to 52ec89a Compare September 8, 2021 20:04

facebook-github-bot closed this in b48cd5f Sep 8, 2021

facebook-github-bot added the Merged label Sep 8, 2021

kgpai mentioned this pull request Sep 9, 2021

Remove string encoding enum #183

Closed

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

[OPPRO-173] Make batch size configurable (facebookincubator#32)

bd55a11

rui-mo added a commit to rui-mo/velox that referenced this pull request Aug 12, 2022

[OPPRO-173] Make batch size configurable (facebookincubator#32)

726fd3d

rui-mo added a commit to rui-mo/velox that referenced this pull request Aug 22, 2022

[OPPRO-173] Make batch size configurable (facebookincubator#32)

16ab7dd

rui-mo added a commit to rui-mo/velox that referenced this pull request Sep 7, 2022

[OPPRO-173] Make batch size configurable (facebookincubator#32)

821342e

kagamiori mentioned this pull request Oct 12, 2022

Substrait velox_plan_conversion_test is flaky #2816

Closed

rui-mo pushed a commit to rui-mo/velox that referenced this pull request Mar 17, 2023

[GJ-33] Upgrade substrait and generate substrait plan on driver (face…

71f458a

…bookincubator#32)

mayunlei mentioned this pull request Jun 7, 2023

Coredump caused by modifying the same vector in multiple Driver threads #5169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for ascii encoding for a subset of rows. #32

Support for ascii encoding for a subset of rows. #32

kgpai commented Aug 12, 2021 •

edited

Loading

mbasmanova left a comment

mbasmanova left a comment

mbasmanova Aug 17, 2021

kgpai commented Aug 18, 2021 •

edited

Loading

kgpai Aug 19, 2021

mbasmanova left a comment

mbasmanova Aug 19, 2021

mbasmanova Aug 19, 2021

mbasmanova Aug 19, 2021

kgpai Aug 19, 2021

facebook-github-bot commented Aug 20, 2021

facebook-github-bot commented Aug 20, 2021

facebook-github-bot commented Aug 20, 2021

mbasmanova left a comment

kgpai Aug 21, 2021

kgpai Aug 21, 2021

facebook-github-bot commented Aug 23, 2021

facebook-github-bot commented Aug 23, 2021

mbasmanova left a comment

facebook-github-bot commented Sep 8, 2021

facebook-github-bot commented Sep 8, 2021

		// always then path
		test("if(1=1, lower(C1), lower(C2))", StringEncodingMode::ASCII);

Support for ascii encoding for a subset of rows. #32

Support for ascii encoding for a subset of rows. #32

Conversation

kgpai commented Aug 12, 2021 • edited Loading

mbasmanova left a 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

kgpai commented Aug 18, 2021 • edited Loading

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

facebook-github-bot commented Aug 20, 2021

facebook-github-bot commented Aug 20, 2021

facebook-github-bot commented Aug 20, 2021

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

facebook-github-bot commented Aug 23, 2021

facebook-github-bot commented Aug 23, 2021

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 8, 2021

facebook-github-bot commented Sep 8, 2021

kgpai commented Aug 12, 2021 •

edited

Loading

kgpai commented Aug 18, 2021 •

edited

Loading