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

Fixed UBSan report in MergeTreeIndexSet #9365

Merged
merged 5 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions dbms/src/Disks/DiskSpaceMonitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@

namespace DB
{

namespace ErrorCodes
{
extern const int EXCESSIVE_ELEMENT_IN_CONFIG;
extern const int UNKNOWN_DISK;
extern const int UNKNOWN_POLICY;
extern const int LOGICAL_ERROR;
}


DiskSelector::DiskSelector(const Poco::Util::AbstractConfiguration & config, const String & config_prefix, const Context & context)
{
Poco::Util::AbstractConfiguration::Keys keys;
Expand Down Expand Up @@ -253,10 +263,10 @@ DiskPtr StoragePolicy::getAnyDisk() const
/// StoragePolicy must contain at least one Volume
/// Volume must contain at least one Disk
if (volumes.empty())
throw Exception("StoragePolicy has no volumes. It's a bug.", ErrorCodes::NOT_ENOUGH_SPACE);
throw Exception("StoragePolicy has no volumes. It's a bug.", ErrorCodes::LOGICAL_ERROR);

if (volumes[0]->disks.empty())
throw Exception("Volume '" + volumes[0]->getName() + "' has no disks. It's a bug.", ErrorCodes::NOT_ENOUGH_SPACE);
throw Exception("Volume '" + volumes[0]->getName() + "' has no disks. It's a bug.", ErrorCodes::LOGICAL_ERROR);

return volumes[0]->disks[0];
}
Expand Down
11 changes: 0 additions & 11 deletions dbms/src/Disks/DiskSpaceMonitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,6 @@

namespace DB
{
namespace ErrorCodes
{
extern const int LOGICAL_ERROR;
extern const int NOT_ENOUGH_SPACE;
extern const int NOT_IMPLEMENTED;
extern const int SYSTEM_ERROR;
extern const int UNKNOWN_ELEMENT_IN_CONFIG;
extern const int EXCESSIVE_ELEMENT_IN_CONFIG;
extern const int UNKNOWN_POLICY;
extern const int UNKNOWN_DISK;
}

/// Parse .xml configuration and store information about disks
/// Mostly used for introspection.
Expand Down
1 change: 0 additions & 1 deletion dbms/src/Functions/bitWrapperFunc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ namespace DB

#if USE_EMBEDDED_COMPILER
static constexpr bool compilable = false;

#endif
};

Expand Down
2 changes: 2 additions & 0 deletions dbms/src/Storages/MergeTree/MergeTreeData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ namespace ErrorCodes
extern const int ABORTED;
extern const int UNKNOWN_PART_TYPE;
extern const int UNEXPECTED_AST_STRUCTURE;
extern const int UNKNOWN_DISK;
extern const int NOT_ENOUGH_SPACE;
}


Expand Down
14 changes: 9 additions & 5 deletions dbms/src/Storages/MergeTree/MergeTreeIndexSet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace ErrorCodes
}

/// 0b11 -- can be true and false at the same time
const Field UNKNOWN_FIELD(3u);
static const Field UNKNOWN_FIELD(3u);


MergeTreeIndexGranuleSet::MergeTreeIndexGranuleSet(const MergeTreeIndexSet & index_)
Expand Down Expand Up @@ -236,8 +236,6 @@ MergeTreeIndexConditionSet::MergeTreeIndexConditionSet(
expression_ast = select.where()->clone();
else if (select.prewhere())
expression_ast = select.prewhere()->clone();
else
expression_ast = std::make_shared<ASTLiteral>(UNKNOWN_FIELD);

useless = checkASTUseless(expression_ast);
/// Do not proceed if index is useless for this query.
Expand All @@ -260,6 +258,9 @@ bool MergeTreeIndexConditionSet::alwaysUnknownOrTrue() const

bool MergeTreeIndexConditionSet::mayBeTrueOnGranule(MergeTreeIndexGranulePtr idx_granule) const
{
if (useless)
return true;

auto granule = std::dynamic_pointer_cast<MergeTreeIndexGranuleSet>(idx_granule);
if (!granule)
throw Exception(
Expand Down Expand Up @@ -405,8 +406,11 @@ bool MergeTreeIndexConditionSet::operatorFromAST(ASTPtr & node) const
return true;
}

bool MergeTreeIndexConditionSet::checkASTUseless(const ASTPtr &node, bool atomic) const
bool MergeTreeIndexConditionSet::checkASTUseless(const ASTPtr & node, bool atomic) const
{
if (!node)
return true;

if (const auto * func = node->as<ASTFunction>())
{
if (key_columns.count(func->getColumnName()))
Expand All @@ -422,7 +426,7 @@ bool MergeTreeIndexConditionSet::checkASTUseless(const ASTPtr &node, bool atomic
return checkASTUseless(args[0], atomic);
else
return std::any_of(args.begin(), args.end(),
[this](const auto & arg) { return checkASTUseless(arg, true); });
[this](const auto & arg) { return checkASTUseless(arg, true); });
}
else if (const auto * literal = node->as<ASTLiteral>())
return !atomic && literal->value.get<bool>();
Expand Down
2 changes: 1 addition & 1 deletion dbms/src/Storages/MergeTree/MergeTreeIndexSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class MergeTreeIndexConditionSet : public IMergeTreeIndexCondition
bool atomFromAST(ASTPtr & node) const;
bool operatorFromAST(ASTPtr & node) const;

bool checkASTUseless(const ASTPtr &node, bool atomic = false) const;
bool checkASTUseless(const ASTPtr & node, bool atomic = false) const;

const MergeTreeIndexSet & index;

Expand Down
6 changes: 1 addition & 5 deletions dbms/src/Storages/StorageMergeTree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,8 @@ namespace ErrorCodes
extern const int ABORTED;
extern const int BAD_ARGUMENTS;
extern const int INCORRECT_DATA;
extern const int INCORRECT_FILE_NAME;
extern const int CANNOT_ASSIGN_OPTIMIZE;
extern const int INCOMPATIBLE_COLUMNS;
extern const int PART_IS_TEMPORARILY_LOCKED;
extern const int UNKNOWN_SETTING;
extern const int TOO_BIG_AST;
extern const int NOT_ENOUGH_SPACE;
}

namespace ActionLocks
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
1 1
1 2
1 3
1 4
5 changes: 5 additions & 0 deletions dbms/tests/queries/0_stateless/01087_index_set_ubsan.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
DROP TABLE IF EXISTS t;
create table t (i Int, a Int, s String, index ind_s (s) type set(1) granularity 1) engine = MergeTree order by i;
insert into t values (1, 1, 'a') (2, 1, 'a') (3, 1, 'a') (4, 1, 'a');
SELECT a, i from t ORDER BY a, i;
DROP TABLE t;