Skip to content

Add some multiword data types#11214

Merged
tavplubix merged 7 commits intoClickHouse:masterfrom
Potya:multiword_data_type
Jun 18, 2020
Merged

Add some multiword data types#11214
tavplubix merged 7 commits intoClickHouse:masterfrom
Potya:multiword_data_type

Conversation

@Potya
Copy link
Copy Markdown
Contributor

@Potya Potya commented May 26, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category :

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add support for multi-word data type names (such as DOUBLE PRECISION and CHAR VARYING) for better SQL compatibility.

Related to #10540

@Potya
Copy link
Copy Markdown
Contributor Author

Potya commented May 26, 2020

TODO: tests and mb some new data types

@blinkov blinkov added the pr-improvement Pull request with some product improvements label May 26, 2020
@tavplubix tavplubix self-assigned this May 27, 2020

auto first_word = type->getID();

if (boost::algorithm::to_lower_copy(first_word) == "function_double") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not to move it to ParserIdentifierWithOptionalParameters? Or may be even make separate ParserDataType, because data type parsing becomes too complex with multi-word identifiers. It can be done like the following (it's just a draft and probably will not work or can be done a better way):

ASTPtr type;
ParserIdentifierWithOptionalParameters type_parser;
if (!type_parser.parse(pos, type, expected)
    return false;
/// Cast to ASTFunction looks better then using getID()
ASTFunction & type_func = type->as<ASTFunction>();
if (!type.arguments.empty())
{
    /// It cannot be multi-word type name if it has arguments after the first word
    return true;
}

/// Special cases for compatibility with SQL standard. We can parse several words as type name
/// only for certain first words, otherwise we don't know how many words to parse
if (boost::algorithm::iequals(func.name, "DOUBLE"))
{
    if (ParserKeyword{"PRESICION"}.ignore(pos))
    {
        /// "DOUBLE PRESICION" can be registered in factory.
        /// Also the second word will not be lost in formatImpl(...)
        func.name += " PRESICION";
    }
} 
else if (boost::algorithm::iequals(func.name, "CHAR"))
...
///TODO can multi-word name have parameters? 

@tavplubix
Copy link
Copy Markdown
Member

Stress test failure looks not related: server segfaults on SELECT max(multiply())

@tavplubix tavplubix merged commit bed1f37 into ClickHouse:master Jun 18, 2020
@alexey-milovidov
Copy link
Copy Markdown
Member

@tavplubix It is incomplete:

CREATE TEMPORARY TABLE test (x INT UNSIGNED NOT NULL)

does not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-improvement Pull request with some product improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants