-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[function](cast)Make string casting to integers more like MySQL's behavior #38847
Conversation
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
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.
clang-tidy made some suggestions
|
||
static inline bool is_all_digit(const char* __restrict s, int len) { | ||
for (int i = 0; i < len; ++i) { | ||
if (!LIKELY(s[i] >= '0' && s[i] <= '9')) { |
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.
warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr]
if (!LIKELY(s[i] >= '0' && s[i] <= '9')) {
^
Additional context
be/src/common/compiler_util.h:34: expanded from macro 'LIKELY'
#define LIKELY(expr) __builtin_expect(!!(expr), 1)
^
@@ -306,7 +320,8 @@ | |||
} | |||
val = val * 10 + digit; | |||
} else { | |||
if ((UNLIKELY(i == first || !is_all_whitespace(s + i, len - i)))) { | |||
if ((UNLIKELY(i == first || (!is_all_whitespace(s + i, len - i) && |
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.
warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr]
if ((UNLIKELY(i == first || (!is_all_whitespace(s + i, len - i) &&
^
Additional context
be/src/common/compiler_util.h:35: expanded from macro 'UNLIKELY'
#define UNLIKELY(expr) __builtin_expect(!!(expr), 0)
^
@@ -448,7 +463,8 @@ | |||
T digit = s[i] - '0'; | |||
val = val * 10 + digit; | |||
} else { | |||
if ((UNLIKELY(!is_all_whitespace(s + i, len - i)))) { | |||
if ((UNLIKELY(!is_all_whitespace(s + i, len - i) && |
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.
warning: boolean expression can be simplified by DeMorgan's theorem [readability-simplify-boolean-expr]
if ((UNLIKELY(!is_all_whitespace(s + i, len - i) &&
^
Additional context
be/src/common/compiler_util.h:35: expanded from macro 'UNLIKELY'
#define UNLIKELY(expr) __builtin_expect(!!(expr), 0)
^
run buildall |
TPC-H: Total hot run time: 41877 ms
|
TPC-DS: Total hot run time: 169226 ms
|
ClickBench: Total hot run time: 30.48 s
|
40b3054
to
ec08f35
Compare
run buildall |
TPC-H: Total hot run time: 41735 ms
|
TPC-DS: Total hot run time: 168920 ms
|
ClickBench: Total hot run time: 30.45 s
|
run buildall |
TPC-H: Total hot run time: 41797 ms
|
TPC-DS: Total hot run time: 167928 ms
|
ClickBench: Total hot run time: 29.9 s
|
run buildall |
TPC-H: Total hot run time: 41501 ms
|
TPC-DS: Total hot run time: 169537 ms
|
ClickBench: Total hot run time: 30.11 s
|
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.
LGTM
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
d92dbf4
to
ad92efd
Compare
run buildall |
TPC-H: Total hot run time: 41756 ms
|
TPC-DS: Total hot run time: 169248 ms
|
ClickBench: Total hot run time: 29.89 s
|
TPC-DS: Total hot run time: 169550 ms
|
ClickBench: Total hot run time: 30.15 s
|
26023c5
to
800af2b
Compare
run buildall |
TPC-H: Total hot run time: 39514 ms
|
run buildall |
TPC-H: Total hot run time: 38461 ms
|
TPC-DS: Total hot run time: 196477 ms
|
ClickBench: Total hot run time: 31.54 s
|
run buildall |
TPC-H: Total hot run time: 37891 ms
|
TPC-DS: Total hot run time: 195602 ms
|
ClickBench: Total hot run time: 31.36 s
|
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.
LGTM
PR approved by at least one committer and no changes requested. |
…avior (#38847) ## Proposed changes There are two issues here. First, the results of casting are inconsistent between FE and BE . ``` FE mysql [(none)]>select cast('3.000' as int); +----------------------+ | cast('3.000' as INT) | +----------------------+ | 3 | +----------------------+ mysql [(none)]>set debug_skip_fold_constant = true; BE mysql [(none)]>select cast('3.000' as int); +----------------------+ | cast('3.000' as INT) | +----------------------+ | NULL | +----------------------+ ``` The second issue is that casting on BE converts '3.0' to null. Here, the casting logic for FE and BE has been unified <!--Describe your changes.-->
…avior (apache#38847) ## Proposed changes There are two issues here. First, the results of casting are inconsistent between FE and BE . ``` FE mysql [(none)]>select cast('3.000' as int); +----------------------+ | cast('3.000' as INT) | +----------------------+ | 3 | +----------------------+ mysql [(none)]>set debug_skip_fold_constant = true; BE mysql [(none)]>select cast('3.000' as int); +----------------------+ | cast('3.000' as INT) | +----------------------+ | NULL | +----------------------+ ``` The second issue is that casting on BE converts '3.0' to null. Here, the casting logic for FE and BE has been unified <!--Describe your changes.-->
#41541) …avior (#38847) #38847 ## Proposed changes There are two issues here. First, the results of casting are inconsistent between FE and BE . ``` FE mysql [(none)]>select cast('3.000' as int); +----------------------+ | cast('3.000' as INT) | +----------------------+ | 3 | +----------------------+ mysql [(none)]>set debug_skip_fold_constant = true; BE mysql [(none)]>select cast('3.000' as int); +----------------------+ | cast('3.000' as INT) | +----------------------+ | NULL | +----------------------+ ``` The second issue is that casting on BE converts '3.0' to null. Here, the casting logic for FE and BE has been unified <!--Describe your changes.--> ## Proposed changes Issue Number: close #xxx <!--Describe your changes.--> --------- Co-authored-by: Xinyi Zou <[email protected]>
Proposed changes
There are two issues here. First, the results of casting are inconsistent between FE and BE .
The second issue is that casting on BE converts '3.0' to null. Here, the casting logic for FE and BE has been unified