Skip to content

fix(common): order null as largest by default to align with PostgreSQL #4263

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

Merged
merged 7 commits into from
Jul 28, 2022

Conversation

xiangjinwu
Copy link
Contributor

@xiangjinwu xiangjinwu commented Jul 28, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Without nulls first or nulls last, which we do not support yet, PostgreSQL treats null as largest value, while we have been treating it as the smallest.

  • In memcomparable encoding, null_tag is flipped. That is, null encodes and decodes as 1 rather than 0.
  • In batch sort util, the ordering between null and non-null has been updated to align with PostgreSQL.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

Types of user-facing changes

  • SQL commands, functions, and operators

Release note

When ordering on a column containing null values, our behavior is now same as PostgreSQL. That is, null is treated as larger than any other value.

Refer to a related PR or issue link (optional)

Fixes #3204

@xiangjinwu xiangjinwu added the user-facing-changes Contains changes that are visible to users label Jul 28, 2022
@github-actions github-actions bot added the type/fix Type: Bug fix. Only for pull requests. label Jul 28, 2022
@xiangjinwu xiangjinwu marked this pull request as ready for review July 28, 2022 10:27
@xiangjinwu xiangjinwu requested review from lmatz and stdrc July 28, 2022 10:27
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #4263 (06cc6e4) into main (cab14c2) will decrease coverage by 0.00%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##             main    #4263      +/-   ##
==========================================
- Coverage   74.38%   74.37%   -0.01%     
==========================================
  Files         842      842              
  Lines      121509   121511       +2     
==========================================
- Hits        90381    90379       -2     
- Misses      31128    31132       +4     
Flag Coverage Δ
rust 74.37% <79.16%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/common/src/util/sort_util.rs 91.18% <0.00%> (ø)
src/common/src/types/mod.rs 70.08% <66.66%> (-0.21%) ⬇️
src/batch/src/executor/join/lookup_join.rs 57.97% <100.00%> (ø)
src/common/src/util/ordered/serde.rs 96.21% <100.00%> (ø)
src/meta/src/manager/id.rs 94.56% <0.00%> (-1.09%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Member

@stdrc stdrc left a comment

Choose a reason for hiding this comment

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

LGTM!

@mergify mergify bot merged commit 647de09 into main Jul 28, 2022
@mergify mergify bot deleted the fix-nulls-first-last branch July 28, 2022 12:41
@skyzh
Copy link
Contributor

skyzh commented Jul 28, 2022

Rust’s Option is null-first by default. I guess some BTreeMap cache storing OptionT is still having a wrong behavior. We should take some time to revisit them.

@skyzh
Copy link
Contributor

skyzh commented Jul 28, 2022

#4272

nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
risingwavelabs#4263)

* encode and decode null as largest in memcomparable encoding

* fix unit test except lookup join

```
NEXTEST_EXPERIMENTAL_FILTER_EXPR=1 ./risedev test -E 'not test(~lookup_join)'
```

* update lookup join unit test

* update e2e

* update batch ordering of null

* update e2e

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Type: Bug fix. Only for pull requests. user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default null ordering is opposite of PostgreSQL
4 participants