-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
fix: make nulls-order consistent with postgres #1344
Conversation
#[tokio::test] | ||
async fn test_nulls_first_asc() -> Result<()> { | ||
let mut ctx = ExecutionContext::new(); | ||
let sql = "SELECT * FROM (VALUES (1, 'one'), (2, 'two'), (null, 'three')) AS t (num,letter) ORDER BY num"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every time I construct a test case with a value list, I think of @jimexist lol 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it is so handy
Might be good to add some tests as well to compare it with PostgreSQL (integration-tests folder)? |
OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it out with postgres and indeed this appears consistent
alamb=# select * from A order by a;
a
---
1
2
(3 rows)
I agree with @Dandandan that some integration tests would be nice, though probably not necessary in this case
Done, PTAL! cc @Dandandan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me. Thanks @xudong963
"| 1 | one |", | ||
"+-----+--------+", | ||
]; | ||
assert_batches_eq!(expected, &actual); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Thanks @xudong963 ! |
🎉 |
Which issue does this PR close?
Closes #1343
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
No