Brijesh-Thakkar commented on code in PR #19587:
URL: https://github.com/apache/datafusion/pull/19587#discussion_r2656440736
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -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
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
