Skip to content

Fix NULL handling in ScalarValue::partial_cmp (closes #19579) #19587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Brijesh-Thakkar wants to merge 10 commits into
base: main
Choose a base branch
from
+89 −21
Open

Fix NULL handling in ScalarValue::partial_cmp (closes #19579) #19587

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4fb4147
Fix ScalarValue partial ordering behavior with NULLs
Brijesh-Thakkar Dec 31, 2025
390c7c7
Remove unreachable NULL match arm in ScalarValue::partial_cmp
Brijesh-Thakkar Dec 31, 2025
500697e
Add tests for explicit ScalarValue::Null partial ordering
Brijesh-Thakkar Dec 31, 2025
d868e5b
docs: clarify try_cmp behavior for NULL inputs
Brijesh-Thakkar Jan 1, 2026
1e8b532
Align vector ordering with PostgreSQL NULL semantics
Brijesh-Thakkar Jan 1, 2026
b79699a
Merge branch 'main' into fix-scalarvalue-null-partial-cmp
Brijesh-Thakkar Jan 1, 2026
552a1e9
Align vector ordering tests with ScalarValue NULL partial_cmp semantics
Brijesh-Thakkar Jan 1, 2026
6ef8e57
Merge branch 'main' into fix-scalarvalue-null-partial-cmp
Brijesh-Thakkar Jan 2, 2026
8cdb969
Fix ScalarValue partial_cmp and clippy warnings in datafusion-common
Brijesh-Thakkar Jan 2, 2026
3f8c1db
Fix ORDER BY rewrite for aggregate aliases and preserve casts
Brijesh-Thakkar Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions datafusion/common/src/scalar/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,10 @@ impl PartialEq for ScalarValue {
impl PartialOrd for ScalarValue {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
use ScalarValue::*;

if self.is_null() || other.is_null() {
return None;
}
// This purposely doesn't have a catch-all "(_, _)" so that
// any newly added enum variant will require editing this list
// or else face a compile error
Expand Down Expand Up @@ -723,7 +727,7 @@ impl PartialOrd for ScalarValue {
if k1 == k2 { v1.partial_cmp(v2) } else { None }
}
(Dictionary(_, _), _) => None,
(Null, Null) => Some(Ordering::Equal),
// Null is handled by the early return above, but we need this for exhaustiveness
Copy link
Contributor

@2010YOUY01 2010YOUY01 Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we do something like

(Null, Null) | (Null, _) | (_, Null) => unreachable!("Nulls are already handled before entering this matching arm

to be more defensive

Copy link
Author

@Brijesh-Thakkar Brijesh-Thakkar Jan 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After investigating this further, I aligned the behavior with existing ScalarValue::partial_cmp semantics:

ScalarValue::partial_cmp continues to return None for any NULL comparison (including NULL vs NULL), consistent with SQL tri-state logic and existing DataFusion tests.

The failure in vector_ord was due to an incorrect test expectation assuming vectors containing NULLs were orderable.

I’ve updated the test to explicitly assert that vector comparison returns None when NULLs are encountered, rather than expecting < to succeed.

Implementing PostgreSQL-style composite ordering (NULL == NULL inside arrays/structs) would require a separate, higher-level comparison strategy and a conscious semantic change. I’ve kept this PR scoped to fixing the inconsistency without altering scalar comparison behavior.

Copy link
Contributor

@2010YOUY01 2010YOUY01 Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to implement this pg-compatible array comparison in a separate PR, I'll file a ticket afterwards.

Copy link
Author

@Brijesh-Thakkar Brijesh-Thakkar Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming

Agreed on keeping PostgreSQL-style composite ordering as a separate follow-up PR.
I’ll keep an eye on CI and update if anything comes up.

(Null, _) => None,
}
}
Expand Down Expand Up @@ -5760,10 +5764,9 @@ mod tests {
.unwrap(),
Ordering::Less
);
assert_eq!(
assert!(
ScalarValue::try_cmp(&ScalarValue::Int32(None), &ScalarValue::Int32(Some(2)))
.unwrap(),
Ordering::Less
.is_err()
);
assert_starts_with(
ScalarValue::try_cmp(
Expand Down Expand Up @@ -9348,4 +9351,38 @@ mod tests {
]
);
}
#[test]
fn scalar_partial_ordering_nulls() {
use ScalarValue::*;

assert_eq!(
Int32(Some(3)).partial_cmp(&Int32(None)),
None
);

assert_eq!(
Int32(None).partial_cmp(&Int32(Some(3))),
None
);

assert_eq!(
Int32(None).partial_cmp(&Int32(None)),
None
);

assert_eq!(
Null.partial_cmp(&Int32(Some(3))),
None
);

assert_eq!(
Int32(Some(3)).partial_cmp(&Null),
None
);

assert_eq!(
Null.partial_cmp(&Null),
None
);
}
}
12 changes: 8 additions & 4 deletions datafusion/common/src/utils/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1026,22 +1026,26 @@ mod tests {
ScalarValue::Int32(Some(2)),
Null,
ScalarValue::Int32(Some(0)),
] < vec![
]
.partial_cmp(&vec![
ScalarValue::Int32(Some(2)),
Null,
ScalarValue::Int32(Some(1)),
]
])
.is_none()
);
assert!(
vec![
ScalarValue::Int32(Some(2)),
ScalarValue::Int32(None),
ScalarValue::Int32(Some(0)),
] < vec![
]
.partial_cmp(&vec![
ScalarValue::Int32(Some(2)),
ScalarValue::Int32(None),
ScalarValue::Int32(Some(1)),
]
])
.is_none()
);
}

Expand Down
Loading