fix crash when predicate optimzer & join on #4794
Conversation
| @@ -53,3 +53,5 @@ SELECT \n date, \n id, \n name, \n value, \n b.date, \n b.name | |||
| 2000-01-01 1 test string 1 1 2000-01-01 test string 1 1 | |||
| SELECT \n id, \n date, \n name, \n value\nFROM \n(\n SELECT \n toInt8(1) AS id, \n toDate(\'2000-01-01\') AS date\n FROM system.numbers \n LIMIT 1\n) \nANY LEFT JOIN \n(\n SELECT *\n FROM test.test \n WHERE date = toDate(\'2000-01-01\')\n) AS b USING (date, id)\nWHERE b.date = toDate(\'2000-01-01\') | |||
| 1 2000-01-01 test string 1 1 | |||
| SELECT \n date, \n id, \n name, \n value, \n `b.date`, \n `b.id`, \n `b.name`, \n `b.value`\nFROM \n(\n SELECT \n date, \n id, \n name, \n value, \n b.date, \n b.id, \n b.name, \n b.value\n FROM \n (\n SELECT \n date, \n id, \n name, \n value\n FROM test.test \n WHERE id = 1\n ) AS a \n ANY LEFT JOIN \n (\n SELECT *\n FROM test.test \n ) AS b ON id = b.id\n WHERE id = 1\n) \nWHERE id = 1 | |||
There was a problem hiding this comment.
I'm not sure if b.date should be here.
| @@ -74,7 +74,7 @@ void TranslateQualifiedNamesMatcher::visit(ASTIdentifier & identifier, ASTPtr &, | |||
| IdentifierSemantic::setMembership(identifier, best_table_pos + 1); | |||
|
|
|||
| /// In case if column from the joined table are in source columns, change it's name to qualified. | |||
| if (best_table_pos && data.source_columns.count(identifier.shortName())) | |||
| if (best_table_pos && !data.source_columns.empty() && data.source_columns.count(identifier.shortName())) | |||
There was a problem hiding this comment.
How does it change anything? Does the crash happen inside identifier.shortName()?
There was a problem hiding this comment.
It looks like some HACK. The base line means: if identifier belongs to the second table, we need a long name for it. And it looks correct without changes.
The issue may be in push down where we need to clear semantics for Identifier before pushing it down.
There was a problem hiding this comment.
If you do not need to push long name for Identifier in Push Down subselect, clear IdentifierSemanticImpl.need_long_name there. Or even better, make a new Identifier with the same short name for subselect.
There's a dangerous place in Push Down ( qualifiedName() ) where we get qualified name as compound string. ASTIdentifier now expects that if it is compound it contains correct 2 name parts. The reason of crash, I think, is an attempt to make compound identifier with no parts set and later access to the long identifier name.
|
@4ertus2 OK, I will try to understand and solve the problem you said : ) |
|
Functional tests with address sanitizer have failed at predicate expression optimizer: It makes clear that the fix is not viable. |
zhang2014
force-pushed
the
fix/crash_when_predicate
branch
from
c4a7009
to
0096ef3
Compare
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
For changelog. Remove if this is non-significant change.
Category (leave one):
Short description (up to few sentences):
#3588 (comment)