Skip to content
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 wrong query result when column value is Null #344

Merged
merged 3 commits into from
Nov 26, 2018
Merged

Fix wrong query result when column value is Null #344

merged 3 commits into from
Nov 26, 2018

Conversation

kangkaisen
Copy link
Contributor

Fix 343

The root cause for 343 is ScanKey is wrong.

I1122 18:12:51.577086 21470 olap_scan_node.cpp:380] BuildScanKey
I1122 18:12:51.579259 21470 olap_common.h:200] ScanKeys:
I1122 18:12:51.579268 21470 olap_common.h:203] ScanKey=[3001,3,-128,1090050 : 3001,3,-128,1090050]
I1122 18:12:51.579274 21470 olap_common.h:203] ScanKey=[3001,3,-127,1090050 : 3001,3,-127,1090050]
I1122 18:12:51.579279 21470 olap_common.h:203] ScanKey=[3001,3,-126,1090050 : 3001,3,-126,1090050]


I1122 18:12:51.580550 21470 olap_common.h:203] ScanKey=[3001,3,126,1090050 : 3001,3,126,1090050]
I1122 18:12:51.580555 21470 olap_common.h:203] ScanKey=[3001,3,127,1090050 : 3001,3,127,1090050]

So we should add null to ScanKey when ColumnValueRange convert_to_fixed_value.

I will add a test case to olap_common_test tomorrow

Copy link
Contributor

@imay imay left a comment

Choose a reason for hiding this comment

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

Good patch first.

CREATE TABLE `test_wrong_result` (
  `a` int(11),
  `b` tinyint(4),
  `c` tinyint(4),
  `d` bigint(20)
) ENGINE=OLAP
DUPLICATE KEY(`a`, `b`, `c`,  `d`)
DISTRIBUTED BY HASH(`d`) BUCKETS 10;
SELECT c FROM test_wrong_result WHERE a = 3001 and b = 3 and d = 1090050;

There is something you miss. For your example query, there is no range filter for column c, your patch can work in right way. If there is filter for column c like c > 1 or c <=1, your patch works in a wrong way. Because when column c has any range filter, rows whose c column's value is null would not to be selected.

So, before you assign has_converted to true, you have to check more conditions.

@@ -662,6 +662,8 @@ Status OlapScanKeys::extend_scan_key(ColumnValueRange<T>& range) {
return Status::OK;
}

bool _has_converted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Only member variable's name starts with '_', so change it to has_converted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@morningman morningman changed the title Fix worng query result when column value is Null Fix wrong query result when column value is Null Nov 23, 2018
@kangkaisen
Copy link
Contributor Author

@imay I think if there is filter for column c like c > 1 or c <=1, which will mean c could not be null.

@chaoyli
Copy link
Contributor

chaoyli commented Nov 23, 2018

@kangkaisen I think you are right. When c has predicate, it should not scan NULL data in SQL semantics.

@kangkaisen
Copy link
Contributor Author

When I try running the olap_common_test, there are a lot of compiling errors. I found recently some commits didn't update olap_common_test at the same time.

So I think we should start a new issue to fix olap_common_test compiling error, and then I will add test cases to olap_common_test.

@kangkaisen
Copy link
Contributor Author

This is the olap_common_test issue: 346

@imay
Copy link
Contributor

imay commented Nov 25, 2018

@imay I think if there is filter for column c like c > 1 or c <=1, which will mean c could not be null.

@kangkaisen Yes, c could not be null, however your change will add null to scan_keys

@kangkaisen
Copy link
Contributor Author

@imay I think if there is filter for column c like c > 1 or c <=1, which will mean c could not be null.

@kangkaisen Yes, c could not be null, however your change will add null to scan_keys

@imay Hi, I think this patch won't add null to C column scan_keys. If c = 1 or c > 1, the code won't goto this branch

    } else {
        if (range.is_fixed_value_convertible() && _is_convertible) {
            if (_begin_scan_keys.empty()) {
                if (range.get_convertible_fixed_value_size() < config::doris_max_scan_key_num) {
                    range.convert_to_fixed_value();
                    has_converted = true;
                }
            } else {
                if (range.get_convertible_fixed_value_size() * _begin_scan_keys.size()
                        < config::doris_max_scan_key_num) {
                    range.convert_to_fixed_value();
                    has_converted = true;
                }
            }
        }

@imay
Copy link
Contributor

imay commented Nov 26, 2018

@imay I think if there is filter for column c like c > 1 or c <=1, which will mean c could not be null.

@kangkaisen Yes, c could not be null, however your change will add null to scan_keys

@imay Hi, I think this patch won't add null to C column scan_keys. If c = 1 or c > 1, the code won't goto this branch

    } else {
        if (range.is_fixed_value_convertible() && _is_convertible) {
            if (_begin_scan_keys.empty()) {
                if (range.get_convertible_fixed_value_size() < config::doris_max_scan_key_num) {
                    range.convert_to_fixed_value();
                    has_converted = true;
                }
            } else {
                if (range.get_convertible_fixed_value_size() * _begin_scan_keys.size()
                        < config::doris_max_scan_key_num) {
                    range.convert_to_fixed_value();
                    has_converted = true;
                }
            }
        }

If c < 0 and its ColumnValueRange.is_fixed_value_range will return false, then this branch will be executed

@kangkaisen
Copy link
Contributor Author

@imay I see. I am sorry. I will update the commit.

@kangkaisen
Copy link
Contributor Author

Update the commit.

Copy link
Contributor

@imay imay left a comment

Choose a reason for hiding this comment

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

lgtm

@imay imay merged commit 33873f2 into apache:master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Critical bug: query result is wrong when Column is Null
3 participants