Specialize Primitive Cursor -- make sorts / merges on a single primitive column faster #5897

tustvold · 2023-04-06T17:00:21Z

Which issue does this PR close?

Part of #5882

Rationale for this change

merge sorted i64        time:   [5.3681 ms 5.3730 ms 5.3784 ms]
                        change: [-43.762% -43.690% -43.621%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  3 (3.00%) high mild
  1 (1.00%) high severe

sort merge i64          time:   [5.6187 ms 5.6264 ms 5.6344 ms]
                        change: [-42.354% -42.259% -42.162%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

merge sorted f64        time:   [5.6240 ms 5.6307 ms 5.6386 ms]
                        change: [-40.730% -40.639% -40.548%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

sort merge f64          time:   [5.9045 ms 5.9124 ms 5.9207 ms]
                        change: [-39.173% -39.065% -38.945%] (p = 0.00 < 0.05)
                        Performance has improved.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

tustvold · 2023-04-06T17:02:14Z

datafusion/core/src/physical_plan/sorts/cursor.rs

+    offset: usize,
+    // If nulls first, the first non-null index
+    // Otherwise, the first null index
+    null_threshold: usize,


I'm quite pleased with this formulation of nulls, it avoids needing to consult the null mask at all 😄

Is there any reason it is called null_threshold rather than null_index given it is a null index 🤔

I thought null_index might be confused for the index of the first null, which is not always the case

that makes sense.

tustvold · 2023-04-06T17:03:00Z

datafusion/core/src/physical_plan/sorts/cursor.rs

+    options: SortOptions,
+}
+
+impl<T: ArrowNativeTypeOp> PrimitiveCursor<T> {


The astute will notice this is parameterised on the underlying Native type, helping to reduce codegen by not forcing SortPreservingMergeStream to be generic over ArrowPrimitiveType

tustvold · 2023-04-06T17:03:22Z

datafusion/core/Cargo.toml

+arrow-schema = { workspace = true }
+arrow-array = { workspace = true }


The downcast macros require these crates to be in scope

alamb

Thanks @tustvold -- this looks great. cc @jaylmiller and @ozankabak

alamb · 2023-04-10T17:37:45Z

datafusion/core/src/physical_plan/sorts/cursor.rs

+    offset: usize,
+    // If nulls first, the first non-null index
+    // Otherwise, the first null index
+    null_threshold: usize,


Is there any reason it is called null_threshold rather than null_index given it is a null index 🤔

alamb · 2023-04-10T17:41:22Z

datafusion/core/src/physical_plan/sorts/cursor.rs

+                let s_v = self.value();
+                let o_v = other.value();
+
+                match self.options.descending {


I wonder if it would be worth specializing on consts NULLS_FIRST and ASC/DESC as well to avoid what looks like runtime overhead in the hot path.

Maybe it doesn't make any practical difference

It's definitely something we could experiment, my expectation is that it won't make a huge difference given how branch heavy merging inherently is

alamb · 2023-04-10T17:48:13Z

datafusion/core/src/physical_plan/sorts/merge.rs

    batch_size: usize,
 ) -> Result<SendableRecordBatchStream> {
-    let streams = RowCursorStream::try_new(schema.as_ref(), expressions, streams)?;
+    if expressions.len() == 1 {


I think it is worth a comment here explaining the rationale for this for future readers. Something like

Suggested change

if expressions.len() == 1 {

// special case single column primitives to avoid overhead of runtime comparators

if expressions.len() == 1 {

alamb · 2023-04-10T17:50:33Z

datafusion/core/src/physical_plan/sorts/stream.rs

    }
 }
+
+pub struct PrimitiveCursorStream<T: ArrowPrimitiveType> {


Suggested change

pub struct PrimitiveCursorStream<T: ArrowPrimitiveType> {

/// Specialized stream for sorts on single primitive columns

pub struct PrimitiveCursorStream<T: ArrowPrimitiveType> {

ozankabak · 2023-04-10T18:15:21Z

Thanks @tustvold

…ive column faster (apache#5897) * Specialize PrimitiveCursor (apache#5882) * Toml format * Review feedback

github-actions bot added the core Core DataFusion crate label Apr 6, 2023

tustvold commented Apr 6, 2023

View reviewed changes

Specialize PrimitiveCursor (apache#5882)

04567b7

tustvold force-pushed the primitive-cursor branch from 5f7a3d6 to 04567b7 Compare April 7, 2023 15:05

tustvold marked this pull request as ready for review April 7, 2023 15:05

Toml format

1bd4efa

alamb changed the title ~~Specialize Primitive Cursor~~ Specialize Primitive Cursor -- make sorts / merges on a single column faster Apr 10, 2023

alamb approved these changes Apr 10, 2023

View reviewed changes

alamb changed the title ~~Specialize Primitive Cursor -- make sorts / merges on a single column faster~~ Specialize Primitive Cursor -- make sorts / merges on a single primitive column faster Apr 10, 2023

Review feedback

0cacc4f

tustvold merged commit 4675a58 into apache:main Apr 11, 2023

korowa pushed a commit to korowa/arrow-datafusion that referenced this pull request Apr 13, 2023

Specialize Primitive Cursor -- make sorts / merges on a single primit…

f869f0f

…ive column faster (apache#5897) * Specialize PrimitiveCursor (apache#5882) * Toml format * Review feedback

		arrow-schema = { workspace = true }
		arrow-array = { workspace = true }

	if expressions.len() == 1 {
	// special case single column primitives to avoid overhead of runtime comparators
	if expressions.len() == 1 {

	pub struct PrimitiveCursorStream<T: ArrowPrimitiveType> {
	/// Specialized stream for sorts on single primitive columns
	pub struct PrimitiveCursorStream<T: ArrowPrimitiveType> {

Specialize Primitive Cursor -- make sorts / merges on a single primitive column faster #5897

Specialize Primitive Cursor -- make sorts / merges on a single primitive column faster #5897

Uh oh!

Conversation

tustvold commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozankabak commented Apr 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tustvold commented Apr 6, 2023 •

edited

Loading