-
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
[Enhancement](function)make SUBSTRING_INDEX function DEPEND_ON_ARGUMENT #30392
Conversation
run buildall |
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
TPC-H: Total hot run time: 38626 ms
|
TPC-DS: Total hot run time: 187356 ms
|
ClickBench: Total hot run time: 31.11 s
|
Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
|
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 anyone and no changes requested. |
run p0 |
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
run buildall |
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
} | ||
|
||
bool use_default_implementation_for_nulls() const override { return true; } | ||
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, |
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: function 'execute_impl' has cognitive complexity of 86 (threshold 50) [readability-function-cognitive-complexity]
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
^
Additional context
be/src/vec/functions/function_string.h:1888: +1, including nesting penalty of 0, nesting level increased to 1
if (part_number == 0 || delimiter_size == 0) {
^
be/src/vec/functions/function_string.h:1888: +1
if (part_number == 0 || delimiter_size == 0) {
^
be/src/vec/functions/function_string.h:1889: +2, including nesting penalty of 1, nesting level increased to 2
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:1892: +1, nesting level increased to 1
} else if (part_number > 0) {
^
be/src/vec/functions/function_string.h:1893: +2, including nesting penalty of 1, nesting level increased to 2
if (delimiter_size == 1) {
^
be/src/vec/functions/function_string.h:1895: +3, including nesting penalty of 2, nesting level increased to 3
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:1899: +4, including nesting penalty of 3, nesting level increased to 4
while (num < part_number) {
^
be/src/vec/functions/function_string.h:1903: +5, including nesting penalty of 4, nesting level increased to 5
if (pos != nullptr) {
^
be/src/vec/functions/function_string.h:1906: +1, nesting level increased to 5
} else {
^
be/src/vec/functions/function_string.h:1908: +6, including nesting penalty of 5, nesting level increased to 6
num = (num == 0) ? 0 : num + 1;
^
be/src/vec/functions/function_string.h:1913: +4, including nesting penalty of 3, nesting level increased to 4
if (num == part_number) {
^
be/src/vec/functions/function_string.h:1918: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:1923: +1, nesting level increased to 2
} else {
^
be/src/vec/functions/function_string.h:1926: +3, including nesting penalty of 2, nesting level increased to 3
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:1930: +4, including nesting penalty of 3, nesting level increased to 4
while (num < part_number) {
^
be/src/vec/functions/function_string.h:1934: +5, including nesting penalty of 4, nesting level increased to 5
if (pos < str.data + str.size) {
^
be/src/vec/functions/function_string.h:1937: +1, nesting level increased to 5
} else {
^
be/src/vec/functions/function_string.h:1939: +6, including nesting penalty of 5, nesting level increased to 6
num = (num == 0) ? 0 : num + 1;
^
be/src/vec/functions/function_string.h:1944: +4, including nesting penalty of 3, nesting level increased to 4
if (num == part_number) {
^
be/src/vec/functions/function_string.h:1949: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:1955: +1, nesting level increased to 1
} else {
^
be/src/vec/functions/function_string.h:1958: +2, including nesting penalty of 1, nesting level increased to 2
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:1965: +3, including nesting penalty of 2, nesting level increased to 3
while (num <= part_number && offset >= 0) {
^
be/src/vec/functions/function_string.h:1965: +1
while (num <= part_number && offset >= 0) {
^
be/src/vec/functions/function_string.h:1967: +4, including nesting penalty of 3, nesting level increased to 4
if (offset != -1) {
^
be/src/vec/functions/function_string.h:1968: +5, including nesting penalty of 4, nesting level increased to 5
if (++num == part_number) {
^
be/src/vec/functions/function_string.h:1974: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:1978: +3, including nesting penalty of 2, nesting level increased to 3
num = (offset == -1 && num != 0) ? num + 1 : num;
^
be/src/vec/functions/function_string.h:1978: +1
num = (offset == -1 && num != 0) ? num + 1 : num;
^
be/src/vec/functions/function_string.h:1980: +3, including nesting penalty of 2, nesting level increased to 3
if (num == part_number) {
^
be/src/vec/functions/function_string.h:1981: +4, including nesting penalty of 3, nesting level increased to 4
if (offset == -1) {
^
be/src/vec/functions/function_string.h:1984: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:1990: +1, nesting level increased to 3
} else {
^
} | ||
|
||
bool use_default_implementation_for_nulls() const override { return true; } | ||
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, |
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: function 'execute_impl' exceeds recommended size/complexity thresholds [readability-function-size]
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
^
Additional context
be/src/vec/functions/function_string.h:1862: 136 lines including whitespace and comments (threshold 80)
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
^
String get_name() const override { return name; } | ||
size_t get_number_of_arguments() const override { return 3; } | ||
|
||
DataTypePtr get_return_type_impl(const DataTypes& arguments) const override { |
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: method 'get_return_type_impl' can be made static [readability-convert-member-functions-to-static]
DataTypePtr get_return_type_impl(const DataTypes& arguments) const override { | |
static DataTypePtr get_return_type_impl(const DataTypes& arguments) override { |
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
|
||
DataTypePtr get_return_type_impl(const DataTypes& arguments) const override { | ||
return make_nullable(std::make_shared<DataTypeString>()); | ||
} | ||
|
||
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, |
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: function 'execute_impl' has cognitive complexity of 87 (threshold 50) [readability-function-cognitive-complexity]
Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
^
Additional context
be/src/vec/functions/function_string.h:2029: +1, including nesting penalty of 0, nesting level increased to 1
if (const auto* nullable = check_and_get_column<const ColumnNullable>(*content_column)) {
^
be/src/vec/functions/function_string.h:2048: +1, including nesting penalty of 0, nesting level increased to 1
if (part_number == 0 || delimiter_size == 0) {
^
be/src/vec/functions/function_string.h:2048: +1
if (part_number == 0 || delimiter_size == 0) {
^
be/src/vec/functions/function_string.h:2049: +2, including nesting penalty of 1, nesting level increased to 2
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:2052: +1, nesting level increased to 1
} else if (part_number > 0) {
^
be/src/vec/functions/function_string.h:2053: +2, including nesting penalty of 1, nesting level increased to 2
if (delimiter_size == 1) {
^
be/src/vec/functions/function_string.h:2055: +3, including nesting penalty of 2, nesting level increased to 3
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:2059: +4, including nesting penalty of 3, nesting level increased to 4
while (num < part_number) {
^
be/src/vec/functions/function_string.h:2063: +5, including nesting penalty of 4, nesting level increased to 5
if (pos != nullptr) {
^
be/src/vec/functions/function_string.h:2066: +1, nesting level increased to 5
} else {
^
be/src/vec/functions/function_string.h:2068: +6, including nesting penalty of 5, nesting level increased to 6
num = (num == 0) ? 0 : num + 1;
^
be/src/vec/functions/function_string.h:2073: +4, including nesting penalty of 3, nesting level increased to 4
if (num == part_number) {
^
be/src/vec/functions/function_string.h:2078: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:2083: +1, nesting level increased to 2
} else {
^
be/src/vec/functions/function_string.h:2086: +3, including nesting penalty of 2, nesting level increased to 3
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:2090: +4, including nesting penalty of 3, nesting level increased to 4
while (num < part_number) {
^
be/src/vec/functions/function_string.h:2094: +5, including nesting penalty of 4, nesting level increased to 5
if (pos < str.data + str.size) {
^
be/src/vec/functions/function_string.h:2097: +1, nesting level increased to 5
} else {
^
be/src/vec/functions/function_string.h:2099: +6, including nesting penalty of 5, nesting level increased to 6
num = (num == 0) ? 0 : num + 1;
^
be/src/vec/functions/function_string.h:2104: +4, including nesting penalty of 3, nesting level increased to 4
if (num == part_number) {
^
be/src/vec/functions/function_string.h:2109: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:2115: +1, nesting level increased to 1
} else {
^
be/src/vec/functions/function_string.h:2118: +2, including nesting penalty of 1, nesting level increased to 2
for (size_t i = 0; i < input_rows_count; ++i) {
^
be/src/vec/functions/function_string.h:2125: +3, including nesting penalty of 2, nesting level increased to 3
while (num <= part_number && offset >= 0) {
^
be/src/vec/functions/function_string.h:2125: +1
while (num <= part_number && offset >= 0) {
^
be/src/vec/functions/function_string.h:2127: +4, including nesting penalty of 3, nesting level increased to 4
if (offset != -1) {
^
be/src/vec/functions/function_string.h:2128: +5, including nesting penalty of 4, nesting level increased to 5
if (++num == part_number) {
^
be/src/vec/functions/function_string.h:2134: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:2138: +3, including nesting penalty of 2, nesting level increased to 3
num = (offset == -1 && num != 0) ? num + 1 : num;
^
be/src/vec/functions/function_string.h:2138: +1
num = (offset == -1 && num != 0) ? num + 1 : num;
^
be/src/vec/functions/function_string.h:2140: +3, including nesting penalty of 2, nesting level increased to 3
if (num == part_number) {
^
be/src/vec/functions/function_string.h:2141: +4, including nesting penalty of 3, nesting level increased to 4
if (offset == -1) {
^
be/src/vec/functions/function_string.h:2144: +1, nesting level increased to 4
} else {
^
be/src/vec/functions/function_string.h:2150: +1, nesting level increased to 3
} else {
^
TPC-H: Total hot run time: 37247 ms
|
TeamCity be ut coverage result: |
TPC-DS: Total hot run time: 172750 ms
|
ClickBench: Total hot run time: 30.56 s
|
Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
|
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. |
Proposed changes
Issue Number: #27435
make function SUBSTRING_INDEX DEPEND_ON_ARGUMENT
Further comments
If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...