test: set sortmode rowsort

[xxchan]

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

What's changed and what's your intention?

Problem occured in #3251. After we have parralel batch scan, the results from each partition is not ordered.

I think sorting results by default makes sense. nosort should be added explicitly for special cases.

This PR added control sortmode rowsort to all slt files. Tell me if some files should use nosort

[codecov]

Codecov Report

Merging #3582 (6afd8bb) into main (5ff98d7) will increase coverage by 0.04%.
The diff coverage is 91.69%.

@@            Coverage Diff             @@
##             main    #3582      +/-   ##
==========================================
+ Coverage   74.36%   74.40%   +0.04%     
==========================================
  Files         771      771              
  Lines      108713   108925     +212     
==========================================
+ Hits        80845    81051     +206     
- Misses      27868    27874       +6     

Flag	Coverage Δ
rust	74.40% <91.69%> (+0.04%)	⬆️

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

Impacted Files	Coverage Δ
src/bench/ss_bench/operations/get.rs	0.00% <0.00%> (ø)
...rc/bench/ss_bench/operations/prefix_scan_random.rs	0.00% <0.00%> (ø)
src/ctl/src/cmd_impl/hummock/list_kv.rs	0.00% <0.00%> (ø)
src/meta/src/hummock/compaction_group/mod.rs	95.58% <ø> (+6.92%)	⬆️
src/storage/src/hummock/snapshot_tests.rs	95.00% <ø> (ø)
src/common/src/catalog/mod.rs	62.57% <72.41%> (+2.12%)	⬆️
src/storage/src/keyspace.rs	82.01% <75.00%> (-0.21%)	⬇️
src/storage/src/hummock/state_store_tests.rs	76.23% <80.55%> (-0.02%)	⬇️
src/storage/src/store.rs	64.70% <83.33%> (+10.16%)	⬆️
src/storage/src/hummock/iterator/test_utils.rs	97.05% <95.23%> (-0.48%)	⬇️
... and 17 more

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

[skyzh]

I believe this should not be set globally and should be carefully reviewed to add on specific queries.

Some queries have order-by, which means that they should have order when being scanned out.

[xxchan]

Some queries have order-by, which means that they should have order when being scanned out.

sortmode on single tests will override the global sortmode? Oh indeed, ORDER BY tests can have no sortmode, nosort should be added separately

[xxchan]

I believe this should not be set globally and should be carefully reviewed to add on specific queries.

I think most queries without order by should be rowsort?

[skyzh]

• yes, but one special case that will affect user experience is window table functions / date time columns. Though order is not important in our system, users might feel weird that older windows appear later than newer windows when they select from table. Not sure whether we should make user happy or just tell them we do not ensure order...
• just an idea, maybe we can add a "RW_SORT_PK" mode for batch query so that users can get consistent output when select? We always produce output with pk sorted in batch query if this option is enabled.

[skyzh]

Anyway, I agree that most queries don't require order. We can simply add a nosort to order by.

[xxchan]

BTW, this also reminds me of another problem that parallel scan on ordered mv may have unorderd results? 🤡

[skyzh]

BTW, this also reminds me of another problem that parallel scan on ordered mv may have unorderd results? 🤡

Yes, batch query should do a merge sort after parallel scan.

[skyzh]

One more thing. Do you feel set VARIABLE in sqllogictest is a file-level or statement or a statement affecting global state? Now it's implemented as latter, so if someone do:

include a.slt
include b.slt

If a.slt sets a row sort mode, b.slt will also be affected. If you feel this is counter-intuitive, maybe we can change the behavior of sqllogictest.

[xxchan]

Sounds like a problem, but it didn't occur in our case, because we include .slt.part

Not sure which is better. What if someone wants to include config.slt? 🥸 Slightly prefer file-level variable.

[xxchan]

BTW I'm also considering adding CLI arg to control default sortmode? Also considering strict mode where every query should have an explicit sortmode 😇

[TennyZhuang]

BTW, this also reminds me of another problem that parallel scan on ordered mv may have unorderd results? 🤡

@st1page has some investigations about it.

[BugenZhao]

Some queries have order-by, which means that they should have order when being scanned out.

+1. I believe this is important or user may find that TopN in materialized views make no sense. 🤣 Suggest reverting this PR after merge sort on top of the scans is implemented, then we can discuss which query does not need to be ordered.

[xxchan]

Let's go to #3583 for more discussion if any