fix: Fix grouping key reordering during spilling #12395

zation99 · 2025-02-20T02:16:16Z

Summary:
When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in #11720.

However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures.

This PR fixes by adding a spillDataLoader which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading.

Differential Revision: D69860326

facebook-github-bot · 2025-02-20T02:16:25Z

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

netlify · 2025-02-20T02:16:39Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`d4725b8`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67b78f68ef61d400084e66ed

…2395) Summary: When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in facebookincubator#11720. However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures. This PR fixes by adding a spillDataLoader which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading. Differential Revision: D69860326

facebook-github-bot · 2025-02-20T02:51:09Z

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

…2395) Summary: When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in facebookincubator#11720. However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures. This PR fixes by adding a spillDataLoader which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading. Differential Revision: D69860326

facebook-github-bot · 2025-02-20T04:37:33Z

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

…2395) Summary: When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in facebookincubator#11720. However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures. This PR fixes by adding a spillDataLoader which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading. Differential Revision: D69860326

facebook-github-bot · 2025-02-20T04:57:05Z

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

xiaoxmeng

@zation99 good catch % minors. Thanks!

xiaoxmeng · 2025-02-20T17:14:15Z

velox/exec/GroupingSet.cpp

@@ -1215,6 +1259,8 @@ bool GroupingSet::mergeNextWithoutAggregates(
  // less than 'numDistinctSpillFilesPerPartition_'.
  bool newDistinct{true};
  int32_t numOutputRows{0};
+  prepareSpillDataLoad(maxOutputRows, result);


s/prepareSpillDataLoad/prepareSpillResultWithoutAggregates/

xiaoxmeng · 2025-02-20T17:14:42Z

velox/exec/GroupingSet.cpp

@@ -1239,13 +1285,14 @@ bool GroupingSet::mergeNextWithoutAggregates(
    }
    if (newDistinct) {
      // Yield result for new distinct.
-      result->copy(
+      spillDataLoader_->copy(


s/spillDataLoader_/spillResultWitoutAggregates_/

xiaoxmeng · 2025-02-20T17:18:14Z

velox/exec/GroupingSet.cpp

@@ -1239,13 +1285,14 @@ bool GroupingSet::mergeNextWithoutAggregates(
    }
    if (newDistinct) {
      // Yield result for new distinct.
-      result->copy(
+      spillDataLoader_->copy(
          &stream->current(), numOutputRows++, stream->currentIndex(), 1);
    }
    stream->pop();
    newDistinct = true;
  }
  result->resize(numOutputRows);


spillResultWitoutAggregates_->resize(numOutputRows); restoreResult(result);

I might be wrong but do we need to maintain spillResultWitoutAggregates_, since it won't be used until the next merge where we call prepareForReuse anyway?

velox/exec/GroupingSet.cpp

xiaoxmeng · 2025-02-20T17:19:52Z

velox/exec/GroupingSet.cpp

+void GroupingSet::prepareSpillDataLoad(
+    int32_t maxOutputRows,
+    const RowVectorPtr& result) {
+  if (!spillConfig_->prefixSortEnabled()) {


Let's do this unconditionally

xiaoxmeng · 2025-02-20T17:25:19Z

velox/exec/GroupingSet.cpp

+
+  unsigned int size = result->type()->size();
+  if (spillDataLoader_ == nullptr) {
+    std::vector<std::string> names(size);


I think we shall set the row type once in GroupingSet::createHashTable()

Updated types construction by directly using table_->rows()->keyTypes().

…2395) Summary: When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in facebookincubator#11720. However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures. This PR fixes by adding a spillDataLoader which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading. Differential Revision: D69860326

facebook-github-bot · 2025-02-20T19:27:07Z

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

…2395) Summary: When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in facebookincubator#11720. However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures. This PR fixes by adding a spill data loader (`spillResultWitoutAggregates_`) which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading. Differential Revision: D69860326

facebook-github-bot · 2025-02-20T19:43:27Z

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

xiaoxmeng

@zation99 thanks for the update % minors

xiaoxmeng · 2025-02-20T19:34:17Z

velox/exec/GroupingSet.h

+  // In case of grouping key reordering, spilled data is first loaded into
+  // 'spillResultWitoutAggregates_', which is then reordered back and load to
+  // result.
+  RowVectorPtr spillResultWitoutAggregates_{nullptr};


typo: s/spillResultWitoutAggregates_/spillResultWithoutAggregates_/

xiaoxmeng · 2025-02-20T19:36:27Z

velox/exec/GroupingSet.h

@@ -336,6 +345,11 @@ class GroupingSet {
  // First row in remainingInput_ that needs to be processed.
  vector_size_t firstRemainingRow_;

+  // In case of grouping key reordering, spilled data is first loaded into


// In case of distinct aggregation without aggregates and the grouping key reordered, the spilled data ..., and is then projected for output.

xiaoxmeng · 2025-02-20T19:37:12Z

velox/exec/GroupingSet.h

+
+  // If prefixsort is enabled, loads the read data from spillDataLoader_ into
+  // result.
+  void restoreResult(const RowVectorPtr& result);


void projectResult(const RowVectorPtr& result);

xiaoxmeng · 2025-02-20T19:39:30Z

velox/exec/GroupingSet.h

@@ -189,6 +189,15 @@ class GroupingSet {
  // index for this aggregation), otherwise it returns reference to activeRows_.
  const SelectivityVector& getSelectivityVector(size_t aggregateIndex) const;

+  // Prepare spillDataLoader_ for loading spilled data.


update the comments as we don't use spillDataLoader_?

xiaoxmeng · 2025-02-20T19:40:22Z

velox/exec/GroupingSet.cpp

+void GroupingSet::prepareSpillResultWithoutAggregates(
+    int32_t maxOutputRows,
+    const RowVectorPtr& result) {
+  unsigned int size = result->type()->size();


const auto numColumns = result->type()->size();

xiaoxmeng · 2025-02-20T19:41:46Z

velox/exec/GroupingSet.cpp

+    std::vector<std::string> names(size);
+    std::vector<TypePtr> types{table_->rows()->keyTypes()};
+
+    for (auto i = 0; i < size; ++i) {


const auto& resultType = dynamic_cast(result->type());

and use in the loop

xiaoxmeng · 2025-02-20T19:42:40Z

velox/exec/GroupingSet.cpp

+        maxOutputRows,
+        &pool_);
+  } else {
+    VectorPtr spillDataLoader = std::move(spillResultWitoutAggregates_);


s/spillDataLoader/spillResultWitoutAggregates/

velox/exec/GroupingSet.cpp

xiaoxmeng · 2025-02-20T19:45:15Z

velox/exec/GroupingSet.cpp

          &stream->current(), numOutputRows++, stream->currentIndex(), 1);
    }
    stream->pop();
    newDistinct = true;
  }
+  restoreResult(result);


Let's do

spillResultWitoutAggregates_->resize(numOutputRows); projectResult(result); which resize result based on spillResultWitoutAggregates_ size internally?

Thanks!

…2395) Summary: When prefix sort is enabled, we sort the grouping keys to maximize the prefixsort benefit as introduced in facebookincubator#11720. However, when spilling happens and when reading the spilled data, there are key order mismatch between the spilled data and the operator output. It can cause segmentation fault when there is RowType mismatch or other type mismatch failures. This PR fixes by adding a spill data loader (`spillResultWitoutAggregates_`) which has the reordered grouping keys, loading the spilled data, and mapping the keys back to result after loading. Differential Revision: D69860326

facebook-github-bot · 2025-02-20T20:24:33Z

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

xiaoxmeng

@zation99 LGTM % nit. Thanks!

velox/exec/GroupingSet.cpp

facebook-github-bot · 2025-02-21T08:44:05Z

This pull request has been merged in 4adec18.

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 Feb 20, 2025

facebook-github-bot added the fb-exported label Feb 20, 2025

zation99 force-pushed the export-D69860326 branch from 9306ed1 to d8d8839 Compare February 20, 2025 02:51

zation99 force-pushed the export-D69860326 branch from d8d8839 to d7ded2a Compare February 20, 2025 04:37

zation99 force-pushed the export-D69860326 branch from d7ded2a to a5bdf09 Compare February 20, 2025 04:56

zation99 requested review from xiaoxmeng and tanjialiang February 20, 2025 06:55

xiaoxmeng reviewed Feb 20, 2025

View reviewed changes

zation99 force-pushed the export-D69860326 branch from a5bdf09 to 0bb230f Compare February 20, 2025 19:26

zation99 force-pushed the export-D69860326 branch from 0bb230f to 33b21b9 Compare February 20, 2025 19:43

xiaoxmeng reviewed Feb 20, 2025

View reviewed changes

zation99 force-pushed the export-D69860326 branch from 33b21b9 to d4725b8 Compare February 20, 2025 20:24

xiaoxmeng approved these changes Feb 20, 2025

View reviewed changes

velox/exec/GroupingSet.cpp Show resolved Hide resolved

facebook-github-bot closed this in 4adec18 Feb 21, 2025

facebook-github-bot added the Merged label Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix grouping key reordering during spilling #12395

fix: Fix grouping key reordering during spilling #12395

zation99 commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

netlify bot commented Feb 20, 2025 •

edited

Loading

facebook-github-bot commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

xiaoxmeng left a comment

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

zation99 Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

zation99 Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

xiaoxmeng left a comment

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

xiaoxmeng Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

xiaoxmeng left a comment

facebook-github-bot commented Feb 21, 2025

fix: Fix grouping key reordering during spilling #12395

fix: Fix grouping key reordering during spilling #12395

Conversation

zation99 commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

netlify bot commented Feb 20, 2025 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

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 Feb 20, 2025

facebook-github-bot commented Feb 20, 2025

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

Choose a reason for hiding this comment

facebook-github-bot commented Feb 20, 2025

xiaoxmeng left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 21, 2025

netlify bot commented Feb 20, 2025 •

edited

Loading