-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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_kql_issue_found_by_wingfuzz #59626
Fix_kql_issue_found_by_wingfuzz #59626
Conversation
This commit fix the issues: ClickHouse#59036 ClickHouse#59037 both issues are same reason, the input query exceed the max_query_size, so the condition isEnd() of token is not meet and cause the assertion failure
This is an automated comment for commit 69b9547 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
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.
Apart from these small changes I think the PR looks fine. I haven't checked the tests throrougly, but I assume it is coming from a good source and/or validated with other KQL parsers.
I edited the PR description to hopefully make the PR check happy. |
thanks |
I think we also need test for this bug |
this can be tested manually without "--multiquery", otherwise the ErrorMaxQuerySizeExceeded is not triggered can do WING Fuzz test to see if the bug is fixed or not |
Hi @antaljanosbenjamin , can you do the Wing Fuzz test to verify the issue: thanks test result of 02366_kql_makeseries and 02366_kql_extend show failed because they have random row order, the results are correct |
I can try to check the wingfuzz tests, but it is not clear to me how to reproduce the issue. I have also other tasks, so it might take some time. In the meantime, I think the failing KQL test should be fixed. If the row order is random, we should either remove the test or make the row order deterministic. |
yes, I have the test result updated. |
@antaljanosbenjamin, the failures are not related with this PR now |
@antaljanosbenjamin Best Regards |
src/Parsers/Kusto/ParserKQLQuery.cpp
Outdated
@@ -373,9 +373,9 @@ bool ParserKQLQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) | |||
|
|||
uint16_t bracket_count = 0; | |||
|
|||
while ((pos->type < TokenType::EndOfStream || |
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.
Ok, but it works exactly the same.
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.
before we use pos->isEnd() to validate, but when reach max_query_size, the iterator already passed the end
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.
The code looks the same: bool isValid() { return get().type < TokenType::EndOfStream; }
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.
oh, yes, changed according to @antaljanosbenjamin 's suggestion
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 pos->isEnd()
would be false
in case of TokenType::ErrorMaxQuerySizeExceeded
, thus !pos->isEnd()
wouldn't terminate the loop. Meanwhile pos->isValid()
will be false in case of TokenType::ErrorMaxQuerySizeExceeded
, thus stopping the loop. Do I miss something?
Do you mean that the |
Actually this might a general issue in KQL parsers. If I search for
This might mean that all of these checks in Kusto has to be addresssed:
I am not sure, but it is suspicious. I will try to play with them and find new bugs. |
@kashwy if you want to help, you can also check out what happend when some of these |
it generate result of row in random order, will fixed with a separated PR |
I can change all of them, although |
I miss something in the logic then. Could you please clarify? |
sure. |
But what if other parsers are started to parse a portion of the query? E.g.:
I think what happens here is:
But there is another example:
Here the same thing happens, but with
I think this needs to be fixed. All of the (BTW: I just realized that Alexey's comment is about the tests, but it directed my focus and I think we ended up with a better result). |
sure, I am fixing it |
thank you very much. I did not see errors related with kql. Best regard |
I have rerun the clang-tidy build, because that is required check. After that I think the PR can be merged. |
thanks |
Test failures:
|
795c1a9
into
ClickHouse:master
* Backport #59626 to 24.1: Fix_kql_issue_found_by_wingfuzz * Fix build --------- Co-authored-by: robot-clickhouse <[email protected]> Co-authored-by: János Benjamin Antal <[email protected]>
* Backport #59626 to 23.11: Fix_kql_issue_found_by_wingfuzz * Fix build --------- Co-authored-by: robot-clickhouse <[email protected]> Co-authored-by: János Benjamin Antal <[email protected]>
* Backport #59626 to 23.12: Fix_kql_issue_found_by_wingfuzz * Fix build --------- Co-authored-by: robot-clickhouse <[email protected]> Co-authored-by: János Benjamin Antal <[email protected]>
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix segmentation fault in KQL parser when the input query exceeds the
max_query_size
. Also re-enable the KQL dialect. Fixes #59036 and #59037.This commit fixes the issues:
#59036
#59037
both issues are same reason, the input query exceed the max_query_size, so the condition isEnd() of token is not meet and cause the assertion failure