From 32ea1d2fcb2d5d37b4d69b3123c5f7fe1b85b856 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Wed, 14 Dec 2022 18:59:58 +0100 Subject: [PATCH 1/5] Analyzer expired Context crash fix --- ...1_analyzer_expired_context_crash_fix.reference | 1 + .../02501_analyzer_expired_context_crash_fix.sql | 15 +++++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.reference create mode 100644 tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.sql diff --git a/tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.reference b/tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.reference new file mode 100644 index 00000000000..c3174a11f71 --- /dev/null +++ b/tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.reference @@ -0,0 +1 @@ +0 0 \N diff --git a/tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.sql b/tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.sql new file mode 100644 index 00000000000..b9ec14501bd --- /dev/null +++ b/tests/queries/0_stateless/02501_analyzer_expired_context_crash_fix.sql @@ -0,0 +1,15 @@ +SET allow_experimental_analyzer = 1; + +DROP TABLE IF EXISTS test_table; +CREATE TABLE test_table +( + b Int64, + a Int64, + grp_aggreg AggregateFunction(groupArrayArray, Array(UInt64)) +) ENGINE = MergeTree() ORDER BY a; + +INSERT INTO test_table SELECT 0, 0, groupArrayArrayState([toUInt64(1)]); + +SELECT b, a, JSONLength(grp_aggreg, 100, NULL) FROM test_table SETTINGS optimize_aggregation_in_order = 1; + +DROP TABLE test_table; From 0c7e0be0b6ec4c6157ccde5517601980f3035232 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Thu, 15 Dec 2022 13:03:09 +0100 Subject: [PATCH 2/5] Analyzer support INSERT SELECT --- src/Interpreters/InterpreterInsertQuery.cpp | 68 ++++++++++++++----- .../InterpreterSelectQueryAnalyzer.cpp | 11 +++ .../InterpreterSelectQueryAnalyzer.h | 2 + ...analyzer_insert_select_crash_fix.reference | 3 + ...02502_analyzer_insert_select_crash_fix.sql | 27 ++++++++ 5 files changed, 95 insertions(+), 16 deletions(-) create mode 100644 tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.reference create mode 100644 tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql diff --git a/src/Interpreters/InterpreterInsertQuery.cpp b/src/Interpreters/InterpreterInsertQuery.cpp index a1ffe8abac3..4f9cd9d35c5 100644 --- a/src/Interpreters/InterpreterInsertQuery.cpp +++ b/src/Interpreters/InterpreterInsertQuery.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -63,39 +64,54 @@ InterpreterInsertQuery::InterpreterInsertQuery( StoragePtr InterpreterInsertQuery::getTable(ASTInsertQuery & query) { + auto current_context = getContext(); + if (query.table_function) { const auto & factory = TableFunctionFactory::instance(); - TableFunctionPtr table_function_ptr = factory.get(query.table_function, getContext()); + TableFunctionPtr table_function_ptr = factory.get(query.table_function, current_context); /// If table function needs structure hint from select query /// we can create a temporary pipeline and get the header. if (query.select && table_function_ptr->needStructureHint()) { - InterpreterSelectWithUnionQuery interpreter_select{ - query.select, getContext(), SelectQueryOptions(QueryProcessingStage::Complete, 1)}; - auto tmp_pipeline = interpreter_select.buildQueryPipeline(); - ColumnsDescription structure_hint{tmp_pipeline.getHeader().getNamesAndTypesList()}; + Block header_block; + auto select_query_options = SelectQueryOptions(QueryProcessingStage::Complete, 1); + + if (current_context->getSettingsRef().allow_experimental_analyzer) + { + InterpreterSelectQueryAnalyzer interpreter_select(query.select, select_query_options, current_context); + header_block = interpreter_select.getSampleBlock(); + } + else + { + InterpreterSelectWithUnionQuery interpreter_select{ + query.select, current_context, select_query_options}; + auto tmp_pipeline = interpreter_select.buildQueryPipeline(); + header_block = tmp_pipeline.getHeader(); + } + + ColumnsDescription structure_hint{header_block.getNamesAndTypesList()}; table_function_ptr->setStructureHint(structure_hint); } - return table_function_ptr->execute(query.table_function, getContext(), table_function_ptr->getName(), + return table_function_ptr->execute(query.table_function, current_context, table_function_ptr->getName(), /* cached_columns */ {}, /* use_global_context */ false, /* is_insert_query */true); } if (query.table_id) { - query.table_id = getContext()->resolveStorageID(query.table_id); + query.table_id = current_context->resolveStorageID(query.table_id); } else { /// Insert query parser does not fill table_id because table and /// database can be parameters and be filled after parsing. StorageID local_table_id(query.getDatabase(), query.getTable()); - query.table_id = getContext()->resolveStorageID(local_table_id); + query.table_id = current_context->resolveStorageID(local_table_id); } - return DatabaseCatalog::instance().getTable(query.table_id, getContext()); + return DatabaseCatalog::instance().getTable(query.table_id, current_context); } Block InterpreterInsertQuery::getSampleBlock( @@ -384,16 +400,36 @@ BlockIO InterpreterInsertQuery::execute() new_context->setSettings(new_settings); new_context->setInsertionTable(getContext()->getInsertionTable()); - InterpreterSelectWithUnionQuery interpreter_select{ - query.select, new_context, SelectQueryOptions(QueryProcessingStage::Complete, 1)}; - pipeline = interpreter_select.buildQueryPipeline(); + auto select_query_options = SelectQueryOptions(QueryProcessingStage::Complete, 1); + + if (settings.allow_experimental_analyzer) + { + InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, select_query_options, new_context); + pipeline = std::move(interpreter_select_analyzer).extractQueryPipelineBuilder(); + } + else + { + InterpreterSelectWithUnionQuery interpreter_select{ + query.select, new_context, SelectQueryOptions(QueryProcessingStage::Complete, 1)}; + pipeline = interpreter_select.buildQueryPipeline(); + } } else { - /// Passing 1 as subquery_depth will disable limiting size of intermediate result. - InterpreterSelectWithUnionQuery interpreter_select{ - query.select, getContext(), SelectQueryOptions(QueryProcessingStage::Complete, 1)}; - pipeline = interpreter_select.buildQueryPipeline(); + auto select_query_options = SelectQueryOptions(QueryProcessingStage::Complete, 1); + + if (settings.allow_experimental_analyzer) + { + InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, select_query_options, getContext()); + pipeline = std::move(interpreter_select_analyzer).extractQueryPipelineBuilder(); + } + else + { + /// Passing 1 as subquery_depth will disable limiting size of intermediate result. + InterpreterSelectWithUnionQuery interpreter_select{ + query.select, getContext(), SelectQueryOptions(QueryProcessingStage::Complete, 1)}; + pipeline = interpreter_select.buildQueryPipeline(); + } } pipeline.dropTotalsAndExtremes(); diff --git a/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp b/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp index 076d52cab5e..f70bceeffe9 100644 --- a/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp +++ b/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp @@ -116,6 +116,17 @@ QueryPlan && InterpreterSelectQueryAnalyzer::extractQueryPlan() && return std::move(planner).extractQueryPlan(); } +QueryPipelineBuilder && InterpreterSelectQueryAnalyzer::extractQueryPipelineBuilder() && +{ + planner.buildQueryPlanIfNeeded(); + auto & query_plan = planner.getQueryPlan(); + + QueryPlanOptimizationSettings optimization_settings; + BuildQueryPipelineSettings build_pipeline_settings; + + return std::move(*query_plan.buildQueryPipeline(optimization_settings, build_pipeline_settings).release()); +} + void InterpreterSelectQueryAnalyzer::addStorageLimits(const StorageLimitsList & storage_limits) { planner.addStorageLimits(storage_limits); diff --git a/src/Interpreters/InterpreterSelectQueryAnalyzer.h b/src/Interpreters/InterpreterSelectQueryAnalyzer.h index 4a0346c65bb..afe7e3139ff 100644 --- a/src/Interpreters/InterpreterSelectQueryAnalyzer.h +++ b/src/Interpreters/InterpreterSelectQueryAnalyzer.h @@ -36,6 +36,8 @@ public: QueryPlan && extractQueryPlan() &&; + QueryPipelineBuilder && extractQueryPipelineBuilder() &&; + void addStorageLimits(const StorageLimitsList & storage_limits); bool supportsTransactions() const override { return true; } diff --git a/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.reference b/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.reference new file mode 100644 index 00000000000..e07d6e49e15 --- /dev/null +++ b/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.reference @@ -0,0 +1,3 @@ +0 Value_0 +1 Value_1 +2 Value_2 diff --git a/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql b/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql new file mode 100644 index 00000000000..c3ea5692258 --- /dev/null +++ b/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql @@ -0,0 +1,27 @@ +SET allow_experimental_analyzer = 1; + +DROP TABLE IF EXISTS test_table; +CREATE TABLE test_table +( + id UInt64, + value String +) ENGINE=MergeTree ORDER BY id; + + +INSERT INTO test_table SELECT 0, 'Value_0'; + +DROP TABLE IF EXISTS test_table_data; +CREATE TABLE test_table_data +( + id UInt64, + value String +) ENGINE=MergeTree ORDER BY id; + +INSERT INTO test_table_data VALUES (1, 'Value_1'), (2, 'Value_2'); + +INSERT INTO test_table SELECT id, value FROM test_table_data; + +SELECT id, value FROM test_table ORDER BY id; + +DROP TABLE test_table_data; +DROP TABLE test_table; From 21b288c6208c5185e7438f25571781165a6d0c7e Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Wed, 18 Jan 2023 10:44:40 +0100 Subject: [PATCH 3/5] Fixed build --- src/Interpreters/InterpreterInsertQuery.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Interpreters/InterpreterInsertQuery.cpp b/src/Interpreters/InterpreterInsertQuery.cpp index 4f9cd9d35c5..5978275f9a1 100644 --- a/src/Interpreters/InterpreterInsertQuery.cpp +++ b/src/Interpreters/InterpreterInsertQuery.cpp @@ -80,7 +80,7 @@ StoragePtr InterpreterInsertQuery::getTable(ASTInsertQuery & query) if (current_context->getSettingsRef().allow_experimental_analyzer) { - InterpreterSelectQueryAnalyzer interpreter_select(query.select, select_query_options, current_context); + InterpreterSelectQueryAnalyzer interpreter_select(query.select, current_context, select_query_options); header_block = interpreter_select.getSampleBlock(); } else @@ -404,7 +404,7 @@ BlockIO InterpreterInsertQuery::execute() if (settings.allow_experimental_analyzer) { - InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, select_query_options, new_context); + InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, new_context, select_query_options); pipeline = std::move(interpreter_select_analyzer).extractQueryPipelineBuilder(); } else @@ -420,7 +420,7 @@ BlockIO InterpreterInsertQuery::execute() if (settings.allow_experimental_analyzer) { - InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, select_query_options, getContext()); + InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, getContext(), select_query_options); pipeline = std::move(interpreter_select_analyzer).extractQueryPipelineBuilder(); } else From e067a55b78bd387578b138166d6a3c77c08b47a9 Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Fri, 20 Jan 2023 12:19:16 +0100 Subject: [PATCH 4/5] Fixed tests --- src/Interpreters/InterpreterInsertQuery.cpp | 4 ++-- src/Interpreters/InterpreterSelectQueryAnalyzer.cpp | 4 ++-- src/Interpreters/InterpreterSelectQueryAnalyzer.h | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Interpreters/InterpreterInsertQuery.cpp b/src/Interpreters/InterpreterInsertQuery.cpp index 5978275f9a1..7e4d936e9e8 100644 --- a/src/Interpreters/InterpreterInsertQuery.cpp +++ b/src/Interpreters/InterpreterInsertQuery.cpp @@ -405,7 +405,7 @@ BlockIO InterpreterInsertQuery::execute() if (settings.allow_experimental_analyzer) { InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, new_context, select_query_options); - pipeline = std::move(interpreter_select_analyzer).extractQueryPipelineBuilder(); + pipeline = interpreter_select_analyzer.buildQueryPipeline(); } else { @@ -421,7 +421,7 @@ BlockIO InterpreterInsertQuery::execute() if (settings.allow_experimental_analyzer) { InterpreterSelectQueryAnalyzer interpreter_select_analyzer(query.select, getContext(), select_query_options); - pipeline = std::move(interpreter_select_analyzer).extractQueryPipelineBuilder(); + pipeline = interpreter_select_analyzer.buildQueryPipeline(); } else { diff --git a/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp b/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp index f70bceeffe9..853687918c8 100644 --- a/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp +++ b/src/Interpreters/InterpreterSelectQueryAnalyzer.cpp @@ -116,7 +116,7 @@ QueryPlan && InterpreterSelectQueryAnalyzer::extractQueryPlan() && return std::move(planner).extractQueryPlan(); } -QueryPipelineBuilder && InterpreterSelectQueryAnalyzer::extractQueryPipelineBuilder() && +QueryPipelineBuilder InterpreterSelectQueryAnalyzer::buildQueryPipeline() { planner.buildQueryPlanIfNeeded(); auto & query_plan = planner.getQueryPlan(); @@ -124,7 +124,7 @@ QueryPipelineBuilder && InterpreterSelectQueryAnalyzer::extractQueryPipelineBuil QueryPlanOptimizationSettings optimization_settings; BuildQueryPipelineSettings build_pipeline_settings; - return std::move(*query_plan.buildQueryPipeline(optimization_settings, build_pipeline_settings).release()); + return std::move(*query_plan.buildQueryPipeline(optimization_settings, build_pipeline_settings)); } void InterpreterSelectQueryAnalyzer::addStorageLimits(const StorageLimitsList & storage_limits) diff --git a/src/Interpreters/InterpreterSelectQueryAnalyzer.h b/src/Interpreters/InterpreterSelectQueryAnalyzer.h index afe7e3139ff..0c2465224e7 100644 --- a/src/Interpreters/InterpreterSelectQueryAnalyzer.h +++ b/src/Interpreters/InterpreterSelectQueryAnalyzer.h @@ -36,7 +36,7 @@ public: QueryPlan && extractQueryPlan() &&; - QueryPipelineBuilder && extractQueryPipelineBuilder() &&; + QueryPipelineBuilder buildQueryPipeline(); void addStorageLimits(const StorageLimitsList & storage_limits); From 5146087db1b7a4f8148c63e318e9db807567962e Mon Sep 17 00:00:00 2001 From: Maksim Kita Date: Sat, 21 Jan 2023 12:19:09 +0100 Subject: [PATCH 5/5] Fix style --- src/Interpreters/InterpreterInsertQuery.cpp | 8 +++----- .../02502_analyzer_insert_select_crash_fix.sql | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Interpreters/InterpreterInsertQuery.cpp b/src/Interpreters/InterpreterInsertQuery.cpp index 7e4d936e9e8..62f3e190ef6 100644 --- a/src/Interpreters/InterpreterInsertQuery.cpp +++ b/src/Interpreters/InterpreterInsertQuery.cpp @@ -409,13 +409,13 @@ BlockIO InterpreterInsertQuery::execute() } else { - InterpreterSelectWithUnionQuery interpreter_select{ - query.select, new_context, SelectQueryOptions(QueryProcessingStage::Complete, 1)}; + InterpreterSelectWithUnionQuery interpreter_select(query.select, new_context, select_query_options); pipeline = interpreter_select.buildQueryPipeline(); } } else { + /// Passing 1 as subquery_depth will disable limiting size of intermediate result. auto select_query_options = SelectQueryOptions(QueryProcessingStage::Complete, 1); if (settings.allow_experimental_analyzer) @@ -425,9 +425,7 @@ BlockIO InterpreterInsertQuery::execute() } else { - /// Passing 1 as subquery_depth will disable limiting size of intermediate result. - InterpreterSelectWithUnionQuery interpreter_select{ - query.select, getContext(), SelectQueryOptions(QueryProcessingStage::Complete, 1)}; + InterpreterSelectWithUnionQuery interpreter_select(query.select, getContext(), select_query_options); pipeline = interpreter_select.buildQueryPipeline(); } } diff --git a/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql b/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql index c3ea5692258..4643f65988a 100644 --- a/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql +++ b/tests/queries/0_stateless/02502_analyzer_insert_select_crash_fix.sql @@ -7,7 +7,6 @@ CREATE TABLE test_table value String ) ENGINE=MergeTree ORDER BY id; - INSERT INTO test_table SELECT 0, 'Value_0'; DROP TABLE IF EXISTS test_table_data;