-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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](function) fix unexpected be core in string search function #31312
Conversation
Thank you for your contribution to Apache Doris. |
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
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
TeamCity be ut coverage result: |
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.
LGTM
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
TeamCity be ut coverage result: |
TPC-H: Total hot run time: 40756 ms
|
TPC-DS: Total hot run time: 175568 ms
|
ClickBench: Total hot run time: 31.35 s
|
Load test result on machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
|
@@ -495,6 +495,12 @@ public static boolean isCastMatchAllowed(Function desc, Function candicate) { | |||
return false; | |||
} | |||
} | |||
if (FunctionCallExpr.STRING_SEARCH_FUNCTION_SET.contains(desc.functionName())) { | |||
if (descArgTypes[1].isStringType() && candicateArgTypes[1].isArrayType()) { | |||
// The needles arg of search functions should not be allowed to cast from string. |
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.
why this is not allowed?
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.
@zhiqiang-hhhh That's correct. see testcase.
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.
This is to prohibit casting the needles argument from string
-> array<text>
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.
why cast sting to array leading to core?
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.
because the cast function wrapped a nullable layer above the result type when it encountered a string input.
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
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.
need more discuss. hold on now. we may need a complete fix on planner to prohibit string
to array<string>
. @morrySnow PTAL
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.
Do we need to throw exception when we meeting NULL as argument ?
For this type of case, CK will return nil instead of throwing an error, and Doris are currently returning 0. Do we need to maintain consistency with CK, as this function was migrated from CK? |
Signature of this function in doris suggests that the result is always not nullable, if we changed that, there will be many changes on BE, for example maybe we should set I think its ok to keep current behavior. |
@zclllyybb @morrySnow Do we still need to discuss it? Perhaps you can provide me with an example so that I can fix it in the next PR. |
I think the better way is totally forbid implicit casting from |
Completely forbid implicit casting may affect many behaviors. For example, should we ban the following case? case: implicit casting in insert stmt
And I think implicitly casting string to other type is a tradition in doris (ex. string->(int, date,jsonb)), but unfortunately not in Clickhouse, if we want to change it, we really need more discuss about it, may be I should leave it as an individual task. |
looking forward for more opinion! |
Got it. I will try to forbid the case 1 later, that doesn't require a lot of work. For case 2, we can leave it as an individual task to be discussed. |
@zclllyybb @xy720 . Cast |
@amorynan, @morrySnow Any more details? Do we already support casting string to array with json format now ? |
Proposed changes
Issue Number: close #31311
Keep same behavior with ck:
ck docs
Before:
After:
Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...