Skip to content
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

CREATE TABLE AS table_function() #6057

Merged
merged 16 commits into from Jul 22, 2019
Merged

CREATE TABLE AS table_function() #6057

merged 16 commits into from Jul 22, 2019

Conversation

dimarub2000
Copy link
Contributor

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):

  • New Feature

Short description (up to few sentences):
CREATE TABLE AS table_function() is now possible

@dimarub2000 dimarub2000 requested a review from akuzm July 19, 2019 18:07
@alexey-milovidov alexey-milovidov added the pr-feature Pull request with new product feature label Jul 19, 2019
@@ -231,7 +235,7 @@ void ASTCreateQuery::formatQueryImpl(const FormatSettings & settings, FormatStat
<< (!as_database.empty() ? backQuoteIfNeed(as_database) + "." : "") << backQuoteIfNeed(as_table);
}

if (columns_list)
if (columns_list && !as_table_function)
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be able to parse CREATE table (columns_list) AS table_function(...)
Please add negative test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be able to parse CREATE table (columns_list) AS table_function(...)
Please add negative test.

Test will be added, but this code actually prevents printing a frame with columns in metadata sql file, so it can simply be ATTACH table AS table_function(). Here comes the next question, why was columns_list initialized. (It responds to your last comment on this topic) It is initialized with setColumns() to avoid duplicating some code in initialization of a table. But I am not sure if it is right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to fix it with if clause


ReadWriteBufferFromHTTP buf(columns_info_uri, Poco::Net::HTTPRequest::HTTP_POST, nullptr);

std::string columns_info;
readStringBinary(columns_info, buf);
NamesAndTypesList columns = NamesAndTypesList::parse(columns_info);

auto result = std::make_shared<StorageXDBC>(getDatabaseName(), table_name, schema_name, table_name, ColumnsDescription{columns}, context, helper);
///If table_name was not specified by user, it will have the same name as remote_table_name
std::string local_table_name = (table_name == getName()) ? remote_table_name : table_name;
Copy link
Member

Choose a reason for hiding this comment

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

This logic is strange.

throw Exception("MySQL table " + backQuoteIfNeed(remote_database_name) + "." + backQuoteIfNeed(remote_table_name) + " doesn't exist.", ErrorCodes::UNKNOWN_TABLE);

///If table_name was not specified by user, it will have the same name as remote_table_name
std::string local_table_name = (table_name == getName()) ? remote_table_name : table_name;
Copy link
Member

Choose a reason for hiding this comment

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

.

15
16
17
19
Copy link
Member

Choose a reason for hiding this comment

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

The test is correct but surprising.

@@ -384,7 +386,7 @@ ColumnsDescription InterpreterCreateQuery::setColumns(
indices.indices.push_back(
std::dynamic_pointer_cast<ASTIndexDeclaration>(index->clone()));
}
else if (!create.as_table.empty())
else if (!create.as_table.empty() || create.as_table_function)
Copy link
Member

Choose a reason for hiding this comment

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

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants