Push SortOptions into DynComparator Allowing Nested Comparisons (#5426) #5792

tustvold · 2024-05-21T16:40:15Z

Which issue does this PR close?

Closes #5426
Closes #5199
Closes #5559

Rationale for this change

See tickets

What changes are included in this PR?

This pushes null handling out of LexicographicalComparator and into DynComparator. This not only closes off a potential footgun, but also allows supporting nested types in comparison operators.

This probably needs a few more tests prior to merge, in particular StructArray was not supported by LexicographicalComparator before and therefore needs new tests, but the broad meat of the PR I think is ready for review

Are there any user-facing changes?

tustvold · 2024-05-21T16:42:52Z

arrow-ord/src/ord.rs

+/// # use arrow_ord::cmp;
+/// # use arrow_ord::ord::make_comparator;
+/// # use arrow_schema::{ArrowError, SortOptions};
+/// fn eq(a: &dyn Array, b: &dyn Array) -> Result<BooleanArray, ArrowError> {


I debated adding SortOptions to the existing comparison kernels, but that would not only be breaking change, but is also somewhat incoherent. Whilst I accept postgres and some other systems do this, it is rather surprising, at least to me.

Having them separate also makes it more obvious what has an optimised vectorised kernel and what doesn't

I agree with the API choice 👍

I recommend adding the rationale to the docstring at the stop ("this function does not support overrriding NULL handling to ensure optimized kernels, see make_comparator for more control over ordering)

The docstring talks about comparsions and total order and this function implement eq -- woudl it be better ti implement cmp to better match the text?

jayzhan211 · 2024-05-23T07:12:59Z

arrow-ord/src/ord.rs

-fn compare_boolean(left: &dyn Array, right: &dyn Array) -> DynComparator {
-    let left: BooleanArray = left.as_boolean().clone();
-    let right: BooleanArray = right.as_boolean().clone();
+fn compare_impl<const NULLS_FIRST: bool, const DESCENDING: bool, F>(


What is the benefit over using boolean parameters?

Avoids conditional branches in the comparator closure

I see. 💯

The way I think of this is that the <const ...> template parameters basically force the compiler to make special versions of this function -- so the downside is potentially more code (you now get a new copy of the function for each distinct set of type parameters) but the upside is that each of those copies has more optimization opportunities (like the arguments are constant)

alamb

The code looks good to me -- thank you @tustvold . I think it is a very elegant way to solve the problems

Maybe I missed it, but I didn't see any new tests for comparing structs / lists with nested nullability, nor did I see any changes to existing tests.

I think it is important to add tests to both document precisely what the expected behavior as well as ensure we don't introduce a regression in some future refactot

Can you please also run the benchmarks to ensure this doesn't cause any regressions?

alamb · 2024-05-24T12:28:20Z

arrow-ord/src/ord.rs

-fn compare_boolean(left: &dyn Array, right: &dyn Array) -> DynComparator {
-    let left: BooleanArray = left.as_boolean().clone();
-    let right: BooleanArray = right.as_boolean().clone();
+fn compare_impl<const NULLS_FIRST: bool, const DESCENDING: bool, F>(


The way I think of this is that the <const ...> template parameters basically force the compiler to make special versions of this function -- so the downside is potentially more code (you now get a new copy of the function for each distinct set of type parameters) but the upside is that each of those copies has more optimization opportunities (like the arguments are constant)

alamb · 2024-05-24T12:35:25Z

arrow-ord/src/ord.rs

+///
+/// # Postgres-compatible Nested Comparison
+///
+/// Whilst SQL prescribes ternary logic for nulls, that is comparing a value against a null yields


I find the use of terms ternary logic / total ordering/etc somewhat hard to grok exactly what this is saying

Perhaps we can offer an example here -- like

If you want to compare nested nulls in your structures so that

{ a: 1, b: null } null { a: 2, b: 1 }

Sorts like

{ a: 2, b: 1 } { a: 1, b: null } null

if you specify DESC NULL LAST

(if I got that right)

alamb · 2024-05-24T12:37:01Z

arrow-ord/src/ord.rs

-/// assert_eq!(std::cmp::Ordering::Less, cmp(0, 1));
+/// assert_eq!(cmp(0, 1), Ordering::Less);
+///
+/// let array1 = Int32Array::from(vec![Some(1), None]);


I think making this a separate section / example might be clearer

Suggested change

/// let array1 = Int32Array::from(vec![Some(1), None]);

///

/// # Example ordering with NULL

/// let array1 = Int32Array::from(vec![Some(1), None]);

Also it might be helpful here to add an example of SortOptions that are not the default (to show the difference)

alamb · 2024-05-24T12:39:07Z

arrow-ord/src/ord.rs

+/// # use arrow_ord::cmp;
+/// # use arrow_ord::ord::make_comparator;
+/// # use arrow_schema::{ArrowError, SortOptions};
+/// fn eq(a: &dyn Array, b: &dyn Array) -> Result<BooleanArray, ArrowError> {


I agree with the API choice 👍

I recommend adding the rationale to the docstring at the stop ("this function does not support overrriding NULL handling to ensure optimized kernels, see make_comparator for more control over ordering)

alamb · 2024-05-24T12:40:22Z

arrow-ord/src/ord.rs

+/// # use arrow_ord::cmp;
+/// # use arrow_ord::ord::make_comparator;
+/// # use arrow_schema::{ArrowError, SortOptions};
+/// fn eq(a: &dyn Array, b: &dyn Array) -> Result<BooleanArray, ArrowError> {


The docstring talks about comparsions and total order and this function implement eq -- woudl it be better ti implement cmp to better match the text?

alamb · 2024-05-24T12:54:06Z

arrow-ord/src/ord.rs

 ///
+/// If `nulls_first` is true `NULL` values will be considered less than any non-null value,


I think it might help to point people at the faster kernels too

like "see kernels in ord::cmp for fast kernels

tustvold · 2024-05-28T15:51:07Z

Tests are written but waiting on #5811 (as they found a bug in the row format)

…arator

alamb · 2024-05-28T17:50:24Z

arrow-ord/src/ord.rs

-/// of nested nulls. That is nulls within nested types are either greater than any value,
-/// or less than any value (Spark). This could be implemented as below
+/// Whilst SQL prescribes ternary logic for nulls, that is comparing a value against a NULL yields
+/// a NULL, many systems, including postgres, instead apply a total ordering to comparison of


github-actions bot added the arrow Changes to the arrow crate label May 21, 2024

tustvold commented May 21, 2024

View reviewed changes

tustvold changed the title ~~Push SortOptions into DynComparator (#5426)~~ Push SortOptions into DynComparator Allowing Nested Comparisons (#5426) May 21, 2024

tustvold force-pushed the sort-options-comparator branch from 598c2f1 to 15b180e Compare May 21, 2024 17:02

Push SortOptions into DynComparator (apache#5426)

10ce321

tustvold force-pushed the sort-options-comparator branch from 15b180e to 10ce321 Compare May 21, 2024 17:16

Clippy

caa891e

jayzhan211 reviewed May 23, 2024

View reviewed changes

alamb reviewed May 24, 2024

View reviewed changes

alamb mentioned this pull request May 24, 2024

Refine documentation for unary_mut and binary_mut #5798

Merged

tustvold mentioned this pull request May 28, 2024

Revisit List Row Encoding (#5807) #5811

Merged

tustvold marked this pull request as draft May 28, 2024 15:51

tustvold added 2 commits May 28, 2024 17:52

Merge remote-tracking branch 'upstream/master' into sort-options-comp…

fc7956d

…arator

Review feedback

e7b32c8

tustvold marked this pull request as ready for review May 28, 2024 16:56

Tweak print_row

90bd86e

alamb approved these changes May 28, 2024

View reviewed changes

tustvold merged commit 9828bf0 into apache:master May 28, 2024
21 of 22 checks passed

jayzhan211 mentioned this pull request Jun 3, 2024

DataFrame.except() does not work with structs in schema apache/datafusion#10749

Closed

tustvold mentioned this pull request Jun 3, 2024

arrow-ord: Support sortting StructArray #5559

Closed

alamb mentioned this pull request Jun 26, 2024

Implement eq comparison for StructArray #5960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push SortOptions into DynComparator Allowing Nested Comparisons (#5426) #5792

Push SortOptions into DynComparator Allowing Nested Comparisons (#5426) #5792

tustvold commented May 21, 2024

tustvold May 21, 2024

alamb May 24, 2024

alamb May 24, 2024

jayzhan211 May 23, 2024

tustvold May 23, 2024

jayzhan211 May 23, 2024

alamb May 24, 2024

alamb left a comment

alamb May 24, 2024

alamb May 24, 2024

alamb May 24, 2024

alamb May 24, 2024

alamb May 24, 2024

alamb May 24, 2024

tustvold commented May 28, 2024

alamb May 28, 2024

		///
		/// If `nulls_first` is true `NULL` values will be considered less than any non-null value,

Push SortOptions into DynComparator Allowing Nested Comparisons (#5426) #5792

Push SortOptions into DynComparator Allowing Nested Comparisons (#5426) #5792

Conversation

tustvold commented May 21, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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

alamb 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

tustvold commented May 28, 2024

Choose a reason for hiding this comment