add prefix sort #7230

skadilover · 2023-10-25T05:53:31Z

Introducing PrefixSort to improve the performance of memory sort, see Optimize sort #6766
Supports BIGINT，performance is improved in both single-column and multi-column scenarios，see PrefixSortBenchmark

netlify · 2023-10-25T05:53:36Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`d039ab8`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6581657cc1eeb000081c158a

facebook-github-bot · 2023-10-25T05:53:36Z

Hi @skadilover!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

mbasmanova

@skadilover Thank you for working on this. This PR iw 2K+ lines long. Is there a way to break it up into smaller pieces?

mbasmanova

Some initial comments.

mbasmanova · 2023-11-14T12:19:33Z

velox/core/QueryConfig.h

@@ -336,6 +336,11 @@ class QueryConfig {
  static constexpr const char* kEnableExpressionEvaluationCache =
      "enable_expression_evaluation_cache";

+  static constexpr const char* kEnablePrefixSort = "enable_prefix_sort";


Would you add documentation for these properties to config.rst?

mbasmanova · 2023-11-14T12:20:10Z

velox/exec/PrefixSort.h

+ */
+#pragma once
+
+#include "PrefixSortAlgorithm.h"


Use full paths: "velox/exec/PrefixSortAlgorithm.h".

Ditto other places.

mbasmanova · 2023-11-14T12:20:47Z

velox/exec/PrefixSortEncode.h

+ * limitations under the License.
+ */
+#pragma once
+#include "string.h"


Add empty line after #pragma once. Ditto other places.

mbasmanova · 2023-11-14T12:21:15Z

velox/exec/PrefixSortEncode.h

+#include "velox/common/base/Exceptions.h"
+
+template <size_t SIZE>
+static inline void MemcpyFixed(void* dest, const void* src) {


Please, review coding guidelines and update to comply. Function and method names should start lowercase.

mbasmanova · 2023-11-14T12:21:58Z

velox/exec/PrefixSortEncode.h

+    case 246:
+      return MemsetFixed<246>(ptr, value);
+    case 247:
+      return MemsetFixed<247>(ptr, value);


Consider using a macro to shorten this code.

mbasmanova · 2023-11-14T12:23:12Z

velox/exec/PrefixSortAlgorithm.h

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#pragma once


Perhaps, this code can be introduced in a separate PR with unit tests.

mbasmanova · 2023-11-14T12:23:28Z

velox/exec/PrefixSort.h

+  bool needSortData = false;
+};
+
+class PrefixSort {


Perhaps, this can be another PR with tests.

mbasmanova · 2023-11-14T12:23:45Z

velox/exec/RowContainer.h

@@ -161,6 +161,7 @@ class RowContainer {
 public:
  static constexpr uint64_t kUnlimited = std::numeric_limits<uint64_t>::max();
  using Eraser = std::function<void(folly::Range<char**> rows)>;
+  friend class PrefixSort;


Can we avoid using 'friend'?

This is what I have been struggling with for a long time. The reason for using friend is: PrefixSort needs to get data from RowContainer, but I don't want RowContainer to be dependent on the code of PrefixSort. I understand that the current solution that can take into account both performance and code dependencies can only use friend. Do you have any better suggestions?

@mbasmanova

PrefixSort needs to get data from RowContainer, but I don't want RowContainer to be dependent on the code of PrefixSort

@skadilover I haven't read the code yet, but maybe you can explain a bit what data you need and why can't we have some sort of extractXxx API in RowContainer for the purpose.

@mbasmanova The key point is that: when extracting data, we need to rely on PrefixSort logic (normalize/variable length process ..) , this part of logic will be a little complex. If the interface is provided by RowContainer, it will depend on the logic of PrefixSort.

skadilover · 2023-11-14T12:55:49Z

@skadilover Thank you for working on this. This PR iw 2K+ lines long. Is there a way to break it up into smaller pieces?

@mbasmanova
Because this is the first PR, I hope it can help you have a general understanding of the entire plan. Is it reasonable to split the code in this way and meet the requirements of the community:
pr#1 Algrthm
pr#2 Normalize Encode
pr#3 PrefixSort

mbasmanova · 2023-11-14T13:00:08Z

@skadilover It is indeed helpful to have the full picture, hence, let's keep this PR for reference and create small PRs for review and eventual merge.

mbasmanova · 2023-11-14T13:11:12Z

velox/exec/PrefixSort.h

+class PrefixSort {
+ public:
+  PrefixSort(
+      RowContainer* FOLLY_NONNULL rowContainer,


Do not use FOLLY_NONNULL

mbasmanova · 2023-11-22T13:31:34Z

velox/exec/benchmarks/PrefixSortBenchmark.cpp

+    assert(!data.empty());
+    exec::test::PlanBuilder builder;
+
+    auto& type = data[0]->type()->as<TypeKind::ROW>();


This variable is not used.

mbasmanova · 2023-11-22T13:31:48Z

velox/exec/benchmarks/PrefixSortBenchmark.cpp

+      RowTypePtr type,
+      int64_t numVectors,
+      int32_t numPerVector,
+      int32_t stringCardinality = 1000) {


This parameter is not used.

mbasmanova · 2023-11-22T13:35:59Z

velox/exec/SortBuffer.cpp

+          sortedRows_.begin(),
+          sortedRows_.end(),
+          [this](const char* leftRow, const char* rightRow) {
+            for (vector_size_t index = 0; index < sortCompareFlags_.size();


For a simple case sorting by a few integers, we can optimize this comparison to use memcmp, no? I wonder how would this optimization compare with using PrefixSort. Specifically, I'm curious whether it will give us the most benefits with PrefixSort adding just a little bit of perf boost on top.

Oh, this is the code velox use now see the if body thats PrefixSort impl
if (prefixSortConfig_.has_value())
`

That's right and I assume this is very inefficient. I wonder what happens if we just optimize this comparator. I expect it will be speed things up quite a bit.

stale · 2024-03-18T22:23:46Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

skadilover mentioned this pull request Oct 25, 2023

Optimize sort #6766

Open

skadilover force-pushed the WIP_add_prefix_sort branch 2 times, most recently from 5e06d5b to 8de29fd Compare November 8, 2023 10:45

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 Nov 8, 2023

skadilover force-pushed the WIP_add_prefix_sort branch 21 times, most recently from ce1ffb8 to fca0ef0 Compare November 10, 2023 08:23

skadilover changed the title ~~[WIP] add prefix sort~~ add prefix sort Nov 14, 2023

mbasmanova reviewed Nov 14, 2023

View reviewed changes

skadilover marked this pull request as draft November 15, 2023 05:52

skadilover mentioned this pull request Nov 22, 2023

Add custom quick-sort algorithm to use for efficient sorting of normalized keys #7624

Closed

mbasmanova reviewed Nov 22, 2023

View reviewed changes

skadilover requested a review from mbasmanova November 27, 2023 07:11

skadilover added 2 commits December 19, 2023 05:47

add prefix sort

0b9fa36

rebase to main

a9f14d9

skadilover force-pushed the WIP_add_prefix_sort branch from 761660b to a9f14d9 Compare December 19, 2023 08:18

use listRow to prepare PrefixSort

d039ab8

stale bot added the stale label Mar 18, 2024

stale bot closed this Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add prefix sort #7230

add prefix sort #7230

skadilover commented Oct 25, 2023 •

edited

Loading

netlify bot commented Oct 25, 2023 •

edited

Loading

facebook-github-bot commented Oct 25, 2023

mbasmanova left a comment

mbasmanova left a comment

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 14, 2023

skadilover Nov 14, 2023

mbasmanova Nov 14, 2023

skadilover Nov 14, 2023 •

edited

Loading

skadilover commented Nov 14, 2023

mbasmanova commented Nov 14, 2023

mbasmanova Nov 14, 2023

mbasmanova Nov 22, 2023

mbasmanova Nov 22, 2023

mbasmanova Nov 22, 2023

skadilover Nov 22, 2023

mbasmanova Nov 22, 2023

stale bot commented Mar 18, 2024

add prefix sort #7230

add prefix sort #7230

Conversation

skadilover commented Oct 25, 2023 • edited Loading

netlify bot commented Oct 25, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Oct 25, 2023

Action Required

Process

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

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

skadilover Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

skadilover commented Nov 14, 2023

mbasmanova commented Nov 14, 2023

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

stale bot commented Mar 18, 2024

skadilover commented Oct 25, 2023 •

edited

Loading

netlify bot commented Oct 25, 2023 •

edited

Loading

skadilover Nov 14, 2023 •

edited

Loading