Limit Dictionary to Single Level #10942

oerling · 2024-09-06T18:30:50Z

Modifies wrapInDictionary to flatten indices of a wrapped dictionary iwth the wrapping indices.
Makes lazy loading of a dictionary encoded column to combine the indices with a dictionary wrapper if loading with lazy wrapped in a dictionary.
Adds functions to transpose dictionaries with and without nulls.
Changes NestedLoopJoin and MergeJoin so that they wrap their input only after the wrapping indices are known. Previously these would wrap first and only then fill in the indices.
Checks that we do not come across multiple nested dictionaries.

netlify · 2024-09-06T18:31:06Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`d121f54`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/676362d50e32ee0008cca9b1

facebook-github-bot · 2024-09-06T18:33:07Z

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

facebook-github-bot · 2024-10-22T00:07:41Z

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

facebook-github-bot · 2024-10-23T23:15:57Z

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

facebook-github-bot · 2024-11-06T23:28:09Z

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

assignUser · 2024-11-08T15:46:34Z

CMakeLists.txt

@@ -508,8 +508,7 @@ if(${VELOX_BUILD_TESTING})
  set_source(c-ares)
  resolve_dependency(c-ares)

-  set_source(gRPC)
-  resolve_dependency(gRPC)
+  # set_source(gRPC) resolve_dependency(gRPC)


pedroerp

Please address comments and make sure CI is all green.

facebook-github-bot · 2024-12-13T20:56:44Z

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

xiaoxmeng

@oerling thanks for the update % minors. Thanks!

xiaoxmeng · 2024-12-18T20:29:51Z

velox/vector/DecodedVector.cpp

@@ -177,6 +179,10 @@ void DecodedVector::combineWrappers(
        setBaseData(*values, rows);
        return;
      case VectorEncoding::Simple::DICTIONARY: {
+        if (!wasLazy) {
+          // LOG(ERROR) << "Multilevel dict ";


Remove this log

velox/vector/BaseVector.cpp

xiaoxmeng · 2024-12-18T20:32:55Z

velox/vector/BaseVector.cpp

@@ -1007,6 +1031,98 @@ std::string printIndices(
  return out.str();
 }

+// static
+void BaseVector::transposeIndices(
+    const vector_size_t* base,


s/base/baseIndices/ s/size/wrapSize/ s/indices/wrapIndices/ s/result/resultIndices/

xiaoxmeng · 2024-12-18T20:33:31Z

velox/vector/BaseVector.h

+      vector_size_t size,
+      const vector_size_t* wrapIndices,
+      const uint64_t* wrapNulls,
+      vector_size_t* result,


s/result/resultIndices/

xiaoxmeng · 2024-12-18T20:42:06Z

velox/vector/BaseVector.cpp

+    if (isLazyNotLoaded(*base)) {
+      // It is OK to rewrap a lazy. It is an error to wrap a lazy in multiple
+      // different dictionaries.
+      base->containsLazyAndIsWrapped_ = false;


Any check on this if possible? containsLazyAndIsWrapped_ can never be true?

xiaoxmeng · 2024-12-18T20:42:11Z

velox/vector/BaseVector.cpp

+      base->containsLazyAndIsWrapped_ = false;
+    }
+    auto* rawNulls = vector->rawNulls();
+    if (indices->refCount() > 1) {


xiaoxmeng · 2024-12-18T20:42:44Z

velox/vector/BaseVector.cpp

@@ -154,6 +157,37 @@ VectorPtr BaseVector::wrapInDictionary(
    shouldFlatten = !isLazyNotLoaded(*base) && (base->size() / 8) > size;
  }

+  if (vector->encoding() == VectorEncoding::Simple::DICTIONARY) {
+    auto base = vector->valueVector();


s/base/baseValue/

xiaoxmeng · 2024-12-18T20:44:17Z

velox/vector/BaseVector.cpp

+      // different dictionaries.
+      base->containsLazyAndIsWrapped_ = false;
+    }
+    auto* rawNulls = vector->rawNulls();


s/rawNulls/rawBaseNulls/

velox/vector/BaseVector.cpp

- Modifies wrapInDictionary to flatten indices of a wrapped dictionary iwth the wrapping indices. - Makes lazy loading of a dictionary encoded column to combine the indices with a dictionary wrapper if loading with lazy wrapped in a dictionary. - Adds functions to transpose dictionaries with and without nulls. - Changes NestedLoopJoin and MergeJoin so that they wrap their input only after the wrapping indices are known. Previously these would wrap first and only then fill in the indices. - Checks that we do not come across multiple nested dictionaries.

facebook-github-bot · 2024-12-19T00:07:48Z

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

xiaoxmeng · 2024-12-19T06:42:00Z

velox/core/QueryConfig.h

@@ -819,7 +819,7 @@ class QueryConfig {
  }

  bool validateOutputFromOperators() const {
-    return get<bool>(kValidateOutputFromOperators, false);
+    return get<bool>(kValidateOutputFromOperators, true);


You might want set this back to false before land

xiaoxmeng · 2024-12-19T07:08:30Z

velox/expression/tests/PeeledEncodingTest.cpp

@@ -174,6 +174,8 @@ TEST_P(PeeledEncodingBasicTests, allCommonDictionaryLayers) {
  std::vector<VectorPtr> peeledVectors;
  auto peeledEncoding = PeeledEncoding::peel(
      {input1, input2, input3}, rows, localDecodedVector, true, peeledVectors);
+  ASSERT_TRUE(peeledEncoding == nullptr);


Need to remove before land.

xiaoxmeng · 2024-12-19T07:08:40Z

velox/expression/tests/PeeledEncodingTest.cpp

@@ -191,7 +193,7 @@ TEST_P(PeeledEncodingBasicTests, someCommonDictionaryLayers) {
  //                   Dict1(Dict2(Dict3(Flat2)))
  //    Peeled Vectors: Dict2(Flat), Const1, Dict2(Dict3(Flat2))
  //    Peel: Dict1
-
+  GTEST_SKIP();


xiaoxmeng · 2024-12-19T07:08:46Z

velox/expression/tests/PeeledEncodingTest.cpp

@@ -368,6 +368,7 @@ TEST_P(PeeledEncodingBasicTests, dictionaryLayersHavingNulls) {
  //    Peeled Vectors: DictWithNulls(Flat1), Const1,
  //                    DictWithNulls(Dict3(Flat2))
  //    Peel: DictNoNulls
+  GTEST_SKIP();


xiaoxmeng · 2024-12-19T07:08:52Z

velox/expression/tests/PeeledEncodingTest.cpp

@@ -419,6 +420,7 @@ TEST_P(PeeledEncodingBasicTests, constantResize) {
 }

 TEST_P(PeeledEncodingBasicTests, intermidiateLazyLayer) {
+  GTEST_SKIP();


xiaoxmeng · 2024-12-19T07:38:04Z

velox/exec/OperatorUtils.h

+  // wrap nulls must be applied to all columns.
+  Buffer* nulls;
+
+  // Set of distinct wrappers with its transpose result as second. These are


Update the comment?

xiaoxmeng · 2024-12-19T07:40:19Z

velox/exec/OperatorUtils.cpp

+  if (wrapState.transposeResults.empty()) {
+    wrapState.nulls = wrapNulls.get();
+  } else {
+    VELOX_CHECK(


VELOX_CHECK_EQ

xiaoxmeng · 2024-12-19T07:41:51Z

velox/exec/OperatorUtils.h

 void projectChildren(
    std::vector<VectorPtr>& projectedChildren,
    const RowVectorPtr& src,
    const std::vector<IdentityProjection>& projections,
    int32_t size,
-    const BufferPtr& mapping);
+    const BufferPtr& mapping,
+    WrapState* state = nullptr);


WrapState must be provided so consider to change to & the same as WrapOne

xiaoxmeng · 2024-12-19T07:41:58Z

velox/exec/OperatorUtils.h

@@ -154,7 +185,8 @@ void projectChildren(
    const std::vector<VectorPtr>& src,
    const std::vector<IdentityProjection>& projections,
    int32_t size,
-    const BufferPtr& mapping);
+    const BufferPtr& mapping,
+    WrapState* state = nullptr);


xiaoxmeng · 2024-12-19T07:43:09Z

velox/exec/LocalPlanner.cpp

-        if (auto projectNode =
-                std::dynamic_pointer_cast<const core::ProjectNode>(next)) {
+        std::shared_ptr<const core::ProjectNode> projectNode;
+        if (FLAGS_merge_project &&


Why add gflag here?

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

oerling force-pushed the sdict2-pr branch from 1298d05 to e0ebb03 Compare October 22, 2024 00:05

oerling requested review from assignUser and majetideepak as code owners October 22, 2024 00:05

oerling force-pushed the sdict2-pr branch from e0ebb03 to 39700d5 Compare October 23, 2024 23:10

oerling force-pushed the sdict2-pr branch 2 times, most recently from ddec93a to baae829 Compare November 6, 2024 23:00

assignUser requested changes Nov 8, 2024

View reviewed changes

pedroerp requested changes Nov 8, 2024

View reviewed changes

oerling force-pushed the sdict2-pr branch from baae829 to ff7fec1 Compare December 13, 2024 20:54

oerling force-pushed the sdict2-pr branch from ff7fec1 to 9867cab Compare December 18, 2024 19:36

xiaoxmeng reviewed Dec 18, 2024

View reviewed changes

oerling force-pushed the sdict2-pr branch from 9867cab to d121f54 Compare December 19, 2024 00:03

xiaoxmeng reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit Dictionary to Single Level #10942

Limit Dictionary to Single Level #10942

oerling commented Sep 6, 2024

netlify bot commented Sep 6, 2024 •

edited

Loading

facebook-github-bot commented Sep 6, 2024

facebook-github-bot commented Oct 22, 2024

facebook-github-bot commented Oct 23, 2024

facebook-github-bot commented Nov 6, 2024

assignUser Nov 8, 2024

pedroerp left a comment

facebook-github-bot commented Dec 13, 2024

xiaoxmeng left a comment

xiaoxmeng Dec 18, 2024

xiaoxmeng Dec 18, 2024

xiaoxmeng Dec 18, 2024

xiaoxmeng Dec 18, 2024

xiaoxmeng Dec 18, 2024

xiaoxmeng Dec 18, 2024

xiaoxmeng Dec 18, 2024

facebook-github-bot commented Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

xiaoxmeng Dec 19, 2024

Limit Dictionary to Single Level #10942

Are you sure you want to change the base?

Limit Dictionary to Single Level #10942

Conversation

oerling commented Sep 6, 2024

netlify bot commented Sep 6, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Sep 6, 2024

facebook-github-bot commented Oct 22, 2024

facebook-github-bot commented Oct 23, 2024

facebook-github-bot commented Nov 6, 2024

Choose a reason for hiding this comment

pedroerp left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 13, 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

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 Dec 19, 2024

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

netlify bot commented Sep 6, 2024 •

edited

Loading