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.

Sign up for GitHub

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

Fix early constant folding for isNull/isNotNul and analyzer. #64695

Merged
merged 8 commits into from
Jun 11, 2024
Merged

Fix early constant folding for isNull/isNotNul and analyzer. #64695

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c9ce136
Fix early constant folding for isNull/isNotNul and analyzer.
KochetovNicolai May 31, 2024
c45f574
Merge branch 'master' into fix-early-const-folding-analyzer
KochetovNicolai Jun 3, 2024
c8def45
Fixing tests.
KochetovNicolai Jun 4, 2024
c108c51
Fixing test.
KochetovNicolai Jun 5, 2024
0525eeb
Fixing test.
KochetovNicolai Jun 5, 2024
90fdcaa
Merge branch 'master' into fix-early-const-folding-analyzer
KochetovNicolai Jun 5, 2024
2131877
Fixing other tests.
KochetovNicolai Jun 7, 2024
6baae1d
Merge branch 'master' into fix-early-const-folding-analyzer
KochetovNicolai Jun 10, 2024
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
14 changes: 3 additions & 11 deletions src/DataTypes/DataTypeNullable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,8 @@ DataTypePtr removeNullableOrLowCardinalityNullable(const DataTypePtr & type)
}

bool canContainNull(const IDataType & type)
{
if (type.isNullable() || type.isLowCardinalityNullable() || isDynamic(type))
return true;

if (const auto * variant = typeid_cast<const DataTypeVariant *>(&type))
for (const auto & elem : variant->getVariants())
if (canContainNull(*elem))
return true;
Copy link
Member

Choose a reason for hiding this comment

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

That's an interesting change. Is it because of distributed execution?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Looks like variant just always can be nullable, this is implemented in selector.


return false;
}
{
return type.isNullable() || type.isLowCardinalityNullable() || isDynamic(type) || isVariant(type);
}

}
21 changes: 15 additions & 6 deletions src/Planner/PlannerExpressionAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,8 +539,11 @@ PlannerExpressionsAnalysisResult buildExpressionAnalysisResult(const QueryTreeNo
if (query_node.hasWhere())
{
where_analysis_result_optional = analyzeFilter(query_node.getWhere(), current_output_columns, planner_context, actions_chain);
where_action_step_index_optional = actions_chain.getLastStepIndex();
current_output_columns = actions_chain.getLastStepAvailableOutputColumns();
if (where_analysis_result_optional)
Copy link
Member

Choose a reason for hiding this comment

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

Why can it be "empty"?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are returning std::nullopt if where condition is constant 1.

{
where_action_step_index_optional = actions_chain.getLastStepIndex();
current_output_columns = actions_chain.getLastStepAvailableOutputColumns();
}
}

auto aggregation_analysis_result_optional = analyzeAggregation(query_tree, current_output_columns, planner_context, actions_chain);
Expand All @@ -553,8 +556,11 @@ PlannerExpressionsAnalysisResult buildExpressionAnalysisResult(const QueryTreeNo
if (query_node.hasHaving())
{
having_analysis_result_optional = analyzeFilter(query_node.getHaving(), current_output_columns, planner_context, actions_chain);
having_action_step_index_optional = actions_chain.getLastStepIndex();
current_output_columns = actions_chain.getLastStepAvailableOutputColumns();
if (having_analysis_result_optional)
{
having_action_step_index_optional = actions_chain.getLastStepIndex();
current_output_columns = actions_chain.getLastStepAvailableOutputColumns();
}
}

auto window_analysis_result_optional = analyzeWindow(query_tree, current_output_columns, planner_context, actions_chain);
Expand All @@ -567,8 +573,11 @@ PlannerExpressionsAnalysisResult buildExpressionAnalysisResult(const QueryTreeNo
if (query_node.hasQualify())
{
qualify_analysis_result_optional = analyzeFilter(query_node.getQualify(), current_output_columns, planner_context, actions_chain);
qualify_action_step_index_optional = actions_chain.getLastStepIndex();
current_output_columns = actions_chain.getLastStepAvailableOutputColumns();
if (qualify_analysis_result_optional)
{
qualify_action_step_index_optional = actions_chain.getLastStepIndex();
current_output_columns = actions_chain.getLastStepAvailableOutputColumns();
}
}

auto projection_analysis_result = analyzeProjection(query_node, current_output_columns, planner_context, actions_chain);
Expand Down
12 changes: 8 additions & 4 deletions src/Storages/tests/gtest_transform_query_for_external_database.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,17 +368,21 @@ TEST(TransformQueryForExternalDatabase, Null)

check(state, 1, {"field"},
"SELECT field FROM table WHERE field IS NULL",
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NULL)");
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NULL)",
R"(SELECT "field" FROM "test"."table" WHERE 1 = 0)");
check(state, 1, {"field"},
"SELECT field FROM table WHERE field IS NOT NULL",
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NOT NULL)");
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NOT NULL)",
R"(SELECT "field" FROM "test"."table")");

check(state, 1, {"field"},
"SELECT field FROM table WHERE isNull(field)",
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NULL)");
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NULL)",
R"(SELECT "field" FROM "test"."table" WHERE 1 = 0)");
check(state, 1, {"field"},
"SELECT field FROM table WHERE isNotNull(field)",
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NOT NULL)");
R"(SELECT "field" FROM "test"."table" WHERE "field" IS NOT NULL)",
R"(SELECT "field" FROM "test"."table")");
}

TEST(TransformQueryForExternalDatabase, ToDate)
Expand Down
1 change: 0 additions & 1 deletion tests/queries/0_stateless/01822_union_and_constans_error.reference
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +0,0 @@
1
2 changes: 1 addition & 1 deletion tests/queries/0_stateless/01822_union_and_constans_error.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,6 @@ SELECT isNull(t0.c0) OR COUNT('\n?pVa')
FROM t0
GROUP BY t0.c0
HAVING isNull(isNull(t0.c0))
SETTINGS aggregate_functions_null_for_empty = 1, enable_optimize_predicate_expression = 0;
SETTINGS aggregate_functions_null_for_empty = 1, enable_optimize_predicate_expression = 0 format Null;

drop table if exists t0;
2 changes: 1 addition & 1 deletion tests/queries/0_stateless/02892_orc_filter_pushdown.reference
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ select count(), sum(number) from file('02892.orc', ORC, 'number UInt64, negative
select count(), min(negative_or_null), max(negative_or_null) from file('02892.orc', ORC, 'number UInt64, negative_or_null Int64') where (negative_or_null < -500);
596 -1099 -501
select count(), sum(number) from file('02892.orc', ORC, 'number UInt64, negative_or_null Int64') where indexHint(negative_or_null is null);
1000 499500
0 0
select count(), min(negative_or_null), max(negative_or_null) from file('02892.orc', ORC, 'number UInt64, negative_or_null Int64') where (negative_or_null is null);
0 0 0
select count(), sum(number) from file('02892.orc', ORC, 'number UInt64, negative_or_null Int64') where indexHint(negative_or_null in (0, -1, -10, -100, -1000));
Expand Down
2 changes: 1 addition & 1 deletion tests/queries/0_stateless/03164_early_constant_folding_analyzer.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ ENGINE = MergeTree ORDER BY (date, pull_request_number, commit_sha, check_name,
insert into checks select * from generateRandom() limit 1;


select trimLeft(explain) from (explain SELECT count(1) FROM checks WHERE test_name IS NOT NULL SETTINGS allow_experimental_analyzer = 1, allow_experimental_parallel_reading_from_replicas = 0) where explain like '%ReadFromPreparedSource%';
select trimLeft(explain) from (explain SELECT count(1) FROM checks WHERE test_name IS NOT NULL) where explain like '%ReadFromPreparedSource%' SETTINGS allow_experimental_analyzer = 1, allow_experimental_parallel_reading_from_replicas = 0;
Loading