Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Core/Settings.h
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ class IColumn;
M(Int64, max_partitions_to_read, -1, "Limit the max number of partitions that can be accessed in one query. <= 0 means unlimited.", 0) \
M(Bool, check_query_single_value_result, true, "Return check query result as single 1/0 value", 0) \
M(Bool, allow_drop_detached, false, "Allow ALTER TABLE ... DROP DETACHED PART[ITION] ... queries", 0) \
M(Bool, skip_materialized_view_checking_if_source_table_not_exist, false, "Allow attaching to a materialized view even if dependent tables are inaccessible.", 0) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that an additional setting is required. For me this PR a bug fix that makes behaviour of ATTACH query consistent for the both ordinary and materialized views.

Copy link
Member

Choose a reason for hiding this comment

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

Agree that setting is probably redundant, ok to remove. (Though if not removing it - in my opinion its name should end with _on_attach )

Copy link
Member

Choose a reason for hiding this comment

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

We should always skip the check on ATTACH query, the check makes sense for CREATE only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should always skip the check on ATTACH query, the check makes sense for CREATE only

If I understood you correctly, we should check type compatibility only in cases of CREATE. I have reverted changes and added a simple check that the query is not attached.

\
M(UInt64, postgresql_connection_pool_size, 16, "Connection pool size for PostgreSQL table engine and database engine.", 0) \
M(UInt64, postgresql_connection_pool_wait_timeout, 5000, "Connection pool push/pop timeout on empty pool for PostgreSQL table engine and database engine. By default it will block on empty pool.", 0) \
Expand Down
95 changes: 58 additions & 37 deletions src/Interpreters/InterpreterCreateQuery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ namespace ErrorCodes
extern const int ILLEGAL_COLUMN;
extern const int LOGICAL_ERROR;
extern const int UNKNOWN_DATABASE;
extern const int UNKNOWN_TABLE;
extern const int PATH_ACCESS_DENIED;
extern const int NOT_IMPLEMENTED;
extern const int ENGINE_REQUIRED;
Expand Down Expand Up @@ -1076,6 +1077,62 @@ void InterpreterCreateQuery::assertOrSetUUID(ASTCreateQuery & create, const Data
}
}

void InterpreterCreateQuery::checkTypecompatibleForMaterializeView(const ASTCreateQuery & create)
{
if (StoragePtr to_table = DatabaseCatalog::instance().tryGetTable(
{create.to_table_id.database_name, create.to_table_id.table_name, create.to_table_id.uuid},
getContext()
))
{
Copy link
Member

Choose a reason for hiding this comment

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

A minor suggestion: consider using "early return": https://dev.to/jpswade/return-early-12o5

Block input_block;
try
{
if (getContext()->getSettingsRef().allow_experimental_analyzer)
{
input_block = InterpreterSelectQueryAnalyzer::getSampleBlock(create.select->clone(), getContext());
}
else
{
input_block = InterpreterSelectWithUnionQuery(create.select->clone(),
getContext(),
SelectQueryOptions().analyze()).getSampleBlock();
}
}
catch (const Exception & e)
{
if (getContext()->getSettingsRef().skip_materialized_view_checking_if_source_table_not_exist &&
e.code() == ErrorCodes::UNKNOWN_TABLE && create.attach
)
{
LOG_WARNING(&Poco::Logger::get("InterpreterSelectQuery"), "{}", e.message());
return;
}
else
{
throw;
}
}

Block output_block = to_table->getInMemoryMetadataPtr()->getSampleBlock();

ColumnsWithTypeAndName input_columns;
ColumnsWithTypeAndName output_columns;
for (const auto & input_column : input_block)
{
if (const auto * output_column = output_block.findByName(input_column.name))
{
input_columns.push_back(input_column.cloneEmpty());
output_columns.push_back(output_column->cloneEmpty());
}
}

ActionsDAG::makeConvertingActions(
input_columns,
output_columns,
ActionsDAG::MatchColumnsMode::Position
);
}
}

BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create)
{
Expand Down Expand Up @@ -1203,43 +1260,7 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create)
/// Check type compatible for materialized dest table and select columns
if (create.select && create.is_materialized_view && create.to_table_id)
{
if (StoragePtr to_table = DatabaseCatalog::instance().tryGetTable(
{create.to_table_id.database_name, create.to_table_id.table_name, create.to_table_id.uuid},
getContext()
))
{
Block input_block;

if (getContext()->getSettingsRef().allow_experimental_analyzer)
{
input_block = InterpreterSelectQueryAnalyzer::getSampleBlock(create.select->clone(), getContext());
}
else
{
input_block = InterpreterSelectWithUnionQuery(create.select->clone(),
getContext(),
SelectQueryOptions().analyze()).getSampleBlock();
}

Block output_block = to_table->getInMemoryMetadataPtr()->getSampleBlock();

ColumnsWithTypeAndName input_columns;
ColumnsWithTypeAndName output_columns;
for (const auto & input_column : input_block)
{
if (const auto * output_column = output_block.findByName(input_column.name))
{
input_columns.push_back(input_column.cloneEmpty());
output_columns.push_back(output_column->cloneEmpty());
}
}

ActionsDAG::makeConvertingActions(
input_columns,
output_columns,
ActionsDAG::MatchColumnsMode::Position
);
}
checkTypecompatibleForMaterializeView(create);
}

DatabasePtr database;
Expand Down
2 changes: 2 additions & 0 deletions src/Interpreters/InterpreterCreateQuery.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ class InterpreterCreateQuery : public IInterpreter, WithMutableContext
BlockIO doCreateOrReplaceTable(ASTCreateQuery & create, const InterpreterCreateQuery::TableProperties & properties);
/// Inserts data in created table if it's CREATE ... SELECT
BlockIO fillTableIfNeeded(const ASTCreateQuery & create);
/// Check type compatible for materialized dest table and select columns
void checkTypecompatibleForMaterializeView(const ASTCreateQuery & create);

void assertOrSetUUID(ASTCreateQuery & create, const DatabasePtr & database) const;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
3 some_val
3 9
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
SET send_logs_level = 'fatal';

CREATE TABLE test_table (n Int32, s String) ENGINE MergeTree PARTITION BY n ORDER BY n;

CREATE TABLE mview_backend (n Int32, n2 Int64) ENGINE MergeTree PARTITION BY n ORDER BY n;

CREATE MATERIALIZED VIEW mview TO mview_backend AS SELECT n, n * n AS "n2" FROM test_table;

DROP TABLE test_table;

DETACH TABLE mview;

/* Check that we get an exception with the option. */

SET skip_materialized_view_checking_if_source_table_not_exist = 0;
ATTACH TABLE mview; --{serverError 60}

/* Check that we don't get an exception with the option. */
SET skip_materialized_view_checking_if_source_table_not_exist = 1;
ATTACH TABLE mview;

/* Check if the data in the materialized view is updated after the restore.*/
CREATE TABLE test_table (n Int32, s String) ENGINE MergeTree PARTITION BY n ORDER BY n;

INSERT INTO test_table VALUES (3,'some_val');

SELECT n,s FROM test_table ORDER BY n;
SELECT n,n2 FROM mview ORDER by n;