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

Restrict schema type conversion #5098

Merged
merged 18 commits into from
Dec 28, 2022
Merged

Restrict schema type conversion #5098

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
51fffc4
add a function to check legitimacy
wenhaocs Dec 22, 2022
0e5be9a
fix test
wenhaocs Dec 23, 2022
7b44fe5
change to warning level
wenhaocs Dec 23, 2022
c314b98
handle unknown type
wenhaocs Dec 23, 2022
4b529b8
fix tck
wenhaocs Dec 23, 2022
21576c2
change to log error
wenhaocs Dec 23, 2022
bceb3d2
update test to look better
wenhaocs Dec 25, 2022
e5e8752
lint
wenhaocs Dec 25, 2022
18d01fb
lint
wenhaocs Dec 25, 2022
494dfac
change int32 to int64 in tck
wenhaocs Dec 25, 2022
4c25970
fix test_insert_vertex_and_edge_test_2 tck
wenhaocs Dec 25, 2022
f3cadba
fix tck
wenhaocs Dec 25, 2022
b12d547
fix tck
wenhaocs Dec 27, 2022
5370f9e
fix tck
wenhaocs Dec 27, 2022
a9bb5ea
format
wenhaocs Dec 27, 2022
7fac0a1
fix tck
wenhaocs Dec 27, 2022
62cdd6b
fix tck
wenhaocs Dec 27, 2022
f914b61
Merge branch 'master' into haowen/fix_schema
critical27 Dec 28, 2022
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
48 changes: 48 additions & 0 deletions src/meta/MetaServiceUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,47 @@ nebula::cpp2::ErrorCode backupTable(kvstore::KVStore* kvstore,
std::make_move_iterator(backupTableFiles.end()));
return nebula::cpp2::ErrorCode::SUCCEEDED;
}

bool isLegalTypeConversion(cpp2::ColumnTypeDef from, cpp2::ColumnTypeDef to) {
// If not type change, return true. Fixed string will be handled separately.
if (from.get_type() == to.get_type() &&
from.get_type() != nebula::cpp2::PropertyType::FIXED_STRING) {
return true;
}
// For unset type, always return true
if (from.get_type() == nebula::cpp2::PropertyType::UNKNOWN) {
return true;
}
// fixed string can be converted to string or wider fixed string
if (from.get_type() == nebula::cpp2::PropertyType::FIXED_STRING) {
if (to.get_type() == nebula::cpp2::PropertyType::STRING) {
return true;
} else if (to.get_type() == nebula::cpp2::PropertyType::FIXED_STRING) {
return from.get_type_length() <= to.get_type_length();
} else {
return false;
}
}
// int is only allowed to convert to wider int
if (from.get_type() == nebula::cpp2::PropertyType::INT32) {
return to.get_type() == nebula::cpp2::PropertyType::INT64;
}
if (from.get_type() == nebula::cpp2::PropertyType::INT16) {
return to.get_type() == nebula::cpp2::PropertyType::INT64 ||
to.get_type() == nebula::cpp2::PropertyType::INT32;
}
if (from.get_type() == nebula::cpp2::PropertyType::INT8) {
return to.get_type() == nebula::cpp2::PropertyType::INT64 ||
to.get_type() == nebula::cpp2::PropertyType::INT32 ||
to.get_type() == nebula::cpp2::PropertyType::INT16;
}
// Float is only allowed to convert to double
if (from.get_type() == nebula::cpp2::PropertyType::FLOAT) {
return to.get_type() == nebula::cpp2::PropertyType::DOUBLE;
}
// Forbid all the other conversion, as the old data are too different from the new data.
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we forbid the type conversion from boolean to int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Data are still correct if we allow for bool <-> int conversion. But the data before conversion and after conversion will look different. In a strict manner, we can disallow any conversion that will make data look different. Or if we are not that strict, we can allow bool <-> int conversion. Please clarify. cc. @MuYiYong @critical27

Copy link
Contributor

Choose a reason for hiding this comment

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

prefer strict

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could forbid it this version I think, it will introduce extra work in storage.

return false;
}
} // namespace

nebula::cpp2::ErrorCode MetaServiceUtils::alterColumnDefs(std::vector<cpp2::ColumnDef>& cols,
Expand All @@ -58,6 +99,13 @@ nebula::cpp2::ErrorCode MetaServiceUtils::alterColumnDefs(std::vector<cpp2::Colu
LOG(INFO) << "Column: " << colName << " as ttl_col, change not allowed";
return nebula::cpp2::ErrorCode::E_UNSUPPORTED;
}
if (!isLegalTypeConversion(it->get_type(), col.get_type())) {
LOG(ERROR) << "Update colume type " << colName << " from "
<< apache::thrift::util::enumNameSafe(it->get_type().get_type()) << " to "
<< apache::thrift::util::enumNameSafe(col.get_type().get_type())
<< " is not allowed!";
return nebula::cpp2::ErrorCode::E_UNSUPPORTED;
}
*it = col;
return nebula::cpp2::ErrorCode::SUCCEEDED;
}
Expand Down
Loading