Storages: Fix comparing null in MinMaxIndex

[JinheLin]

What problem does this PR solve?

Issue Number: close #9374

Problem Summary:

The filter of select * from test.t where not (a between NULL and '2024-08-25') will be transformed in TiFlash as not (a between NULL and '2024-08-25') => not (a >= NULL and a <= '2024-08-25') => a < NULL or a > '2024-08-25'.

The problem is how RSOperator and MinMaxIndex handle a < NULL.

As the code below, Less will be transformed into not GreaterEqual:

tiflash/dbms/src/Storages/DeltaMerge/Filter/Less.h

Lines 32 to 37 in 81cd947

	RSResults roughCheck(size_t start_pack, size_t pack_count, const RSCheckParam & param) override
	{
	auto results = minMaxCheckCmp<RoughCheck::CheckGreaterEqual>(start_pack, pack_count, param, attr, value);
	std::transform(results.begin(), results.end(), results.begin(), [](RSResult result) { return !result; });
	return results;
	}

MinMaxIndex execute GreaterEqual and the result for comparing with NULL is RSResult::None.

tiflash/dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp

Lines 422 to 426 in 81cd947

	RSResults MinMaxIndex::checkCmp(size_t start_pack, size_t pack_count, const Field & value, const DataTypePtr & type)
	{
	RSResults results(pack_count, RSResult::None);
	if (value.isNull())
	return results;

not RSResult::None equals to RSResult::All. And PR #9361 will skip filter if the filter result of a Block is RSResult::All.

The root cause is that the result of comparing any value with null is null, returning RSResult::None is incorrect.

What is changed and how it works?

In `MinMaxIndex`, if the input value is null, return `RSResult::NoneNull`.

Handling null input value, in checkCmp:

tiflash/dbms/src/Storages/DeltaMerge/Index/MinMaxIndex.cpp

Lines 422 to 426 in 81cd947

	RSResults MinMaxIndex::checkCmp(size_t start_pack, size_t pack_count, const Field & value, const DataTypePtr & type)
	{
	RSResults results(pack_count, RSResult::None);
	if (value.isNull())
	return results;

Handling null input value, in checkIn:

tiflash/dbms/src/Storages/DeltaMerge/Index/RoughCheck.h

Lines 76 to 78 in 81cd947

	// skip null value
	if (v.isNull())
	continue;

For some filters like checkCmp(null), the result is RSResult::NoneNull and the block can be skipped.
For some filters like not checkCmp(null), the result is RSResult::AllNull and the block will be read and filtered.

In fact, the result checkCmp(null) and not checkCmp(null) should always be null. Maybe we can extend RSResult to support this. However, comparing(>, <, >=, <=, ==, !=, in) with null is not a normal operation in MySQL. So just let it fallback to FilterTransforamAction if neccessary.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

None

[JinheLin]

Just move some results to a smaller scope.

[JaySon-Huang]

why "less"/"lessequal"/"noteuqal" return "AllNull" but others return "NoneNull"?

[JinheLin]

Because Less is transformed to not GreaterEqual in implementation...

GreaterEqual(NULL) will return RSResult::NoneNull as expected.

tiflash/dbms/src/Storages/DeltaMerge/Filter/Less.h

Lines 32 to 37 in d113404

	RSResults roughCheck(size_t start_pack, size_t pack_count, const RSCheckParam & param) override
	{
	auto results = minMaxCheckCmp<RoughCheck::CheckGreaterEqual>(start_pack, pack_count, param, attr, value);
	std::transform(results.begin(), results.end(), results.begin(), [](RSResult result) { return !result; });
	return results;
	}

[JinheLin]

LessEqual is transformed to not Greater.
NotEqual is not Equal.

[JaySon-Huang]

Better add some test case about this: https://dev.mysql.com/doc/refman/8.4/en/working-with-null.html

Two NULL values are regarded as equal in a GROUP BY.
When doing an ORDER BY, NULL values are presented first if you do ORDER BY ... ASC and last if you do ORDER BY ... DESC.

[JinheLin]

Added

[JaySon-Huang]

LGTM

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [JaySon-Huang,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2024-08-27 04:02:50.05592476 +0000 UTC m=+842965.190374874: ☑️ agreed by Lloyd-Pottiger.
• 2024-08-27 08:20:12.678406807 +0000 UTC m=+858407.812856930: ☑️ agreed by JaySon-Huang.