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

Jump to bottom

fix the bug when alter column that doesn't have a default value to no… #5105

Merged
merged 3 commits into from
Dec 26, 2022
Merged

fix the bug when alter column that doesn't have a default value to no… #5105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f113661
fix the bug when alter column that doesn't have a default value to no…
jievince Dec 23, 2022
5e2385c
address comments
jievince Dec 26, 2022
bb9dac2
Merge branch 'master' into fix-alter-not-null
Sophie-Xie Dec 26, 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
11 changes: 9 additions & 2 deletions src/graph/validator/MaintainValidator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ namespace graph {
// Validate columns of schema.
// Check validity of columns and fill to thrift structure.
static Status validateColumns(const std::vector<ColumnSpecification *> &columnSpecs,
meta::cpp2::Schema &schema) {
meta::cpp2::Schema &schema,
bool isAlter = false) {
for (auto &spec : columnSpecs) {
meta::cpp2::ColumnDef column;
auto type = spec->type();
Expand Down Expand Up @@ -59,6 +60,12 @@ static Status validateColumns(const std::vector<ColumnSpecification *> &columnSp
if (!column.nullable_ref().has_value()) {
column.nullable_ref() = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this problem recently as well, do you know why? This is quite implicit...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this problem recently as well, do you know why? This is quite implicit...

Is there any related issue? I don't know which problem you're talking about.

}
// Should report an error when altering the column
// which doesn't have default value to not nullable
if (isAlter && !column.nullable_ref().value() && !column.default_value_ref().has_value()) {
return Status::SemanticError("Column `%s' must have a default value if it's not nullable",
spec->name()->c_str());
}
schema.columns_ref().value().emplace_back(std::move(column));
}
return Status::OK();
Expand Down Expand Up @@ -92,7 +99,7 @@ static StatusOr<std::vector<meta::cpp2::AlterSchemaItem>> validateSchemaOpts(
return Status::SemanticError("Duplicate column name `%s'", spec->name()->c_str());
}
}
NG_LOG_AND_RETURN_IF_ERROR(validateColumns(specs, schema));
NG_LOG_AND_RETURN_IF_ERROR(validateColumns(specs, schema, true));
}

schemaItem.schema_ref() = std::move(schema);
Expand Down
16 changes: 8 additions & 8 deletions tests/tck/features/schema/Comment.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ Feature: Schema Comment
"""
ALTER TAG test_comment_tag comment = <tag_of_person_comment_modified>;
ALTER TAG test_comment_tag ADD (gender string COMMENT 'The gender.');
ALTER TAG test_comment_tag CHANGE (name string NOT NULL);
ALTER TAG test_comment_tag CHANGE (name string NOT NULL DEFAULT "jack");
ALTER TAG test_comment_tag DROP (age);
"""
Then the execution should be successful
Expand All @@ -134,15 +134,15 @@ Feature: Schema Comment
SHOW CREATE tag test_comment_tag;
"""
Then the result should be, in any order:
| Tag | Create Tag |
| "test_comment_tag" | 'CREATE TAG `test_comment_tag` (\n `name` string NOT NULL,\n `gender` string NULL COMMENT "The gender."\n) ttl_duration = 0, ttl_col = "", comment = <tag_of_person_comment_modified>' |
| Tag | Create Tag |
| "test_comment_tag" | 'CREATE TAG `test_comment_tag` (\n `name` string NOT NULL DEFAULT \"jack\",\n `gender` string NULL COMMENT "The gender."\n) ttl_duration = 0, ttl_col = "", comment = <tag_of_person_comment_modified>' |
When executing query:
"""
DESC tag test_comment_tag;
"""
Then the result should be, in any order:
| Field | Type | Null | Default | Comment |
| "name" | "string" | "NO" | EMPTY | EMPTY |
| "name" | "string" | "NO" | "jack" | EMPTY |
| "gender" | "string" | "YES" | EMPTY | "The gender." |
# tag index
When executing query:
Expand Down Expand Up @@ -188,7 +188,7 @@ Feature: Schema Comment
"""
ALTER EDGE test_comment_edge comment = <edge_of_person_comment_modified>;
ALTER EDGE test_comment_edge ADD (gender string COMMENT 'The gender.');
ALTER EDGE test_comment_edge CHANGE (name string NOT NULL);
ALTER EDGE test_comment_edge CHANGE (name string NOT NULL DEFAULT "jack");
ALTER EDGE test_comment_edge DROP (age);
"""
Then the execution should be successful
Expand All @@ -198,15 +198,15 @@ Feature: Schema Comment
SHOW CREATE edge test_comment_edge;
"""
Then the result should be, in any order:
| Edge | Create Edge |
| "test_comment_edge" | 'CREATE EDGE `test_comment_edge` (\n `name` string NOT NULL,\n `gender` string NULL COMMENT "The gender."\n) ttl_duration = 0, ttl_col = "", comment = <edge_of_person_comment_modified>' |
| Edge | Create Edge |
| "test_comment_edge" | 'CREATE EDGE `test_comment_edge` (\n `name` string NOT NULL DEFAULT \"jack\",\n `gender` string NULL COMMENT "The gender."\n) ttl_duration = 0, ttl_col = "", comment = <edge_of_person_comment_modified>' |
When executing query:
"""
DESC edge test_comment_edge;
"""
Then the result should be, in any order:
| Field | Type | Null | Default | Comment |
| "name" | "string" | "NO" | EMPTY | EMPTY |
| "name" | "string" | "NO" | "jack" | EMPTY |
| "gender" | "string" | "YES" | EMPTY | "The gender." |
# edge index
When executing query:
Expand Down
91 changes: 75 additions & 16 deletions tests/tck/features/schema/Schema.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ Feature: Insert string vid of vertex and edge
"""
ALTER TAG t CHANGE (description string NOT NULL)
"""
Then the execution should be successful
Then a SemanticError should be raised at runtime: Column `description' must have a default value if it's not nullable
And wait 3 seconds
# insert
When executing query:
Expand All @@ -532,13 +532,6 @@ Feature: Insert string vid of vertex and edge
Then the result should be, in any order:
| t.name | t.age | t.description |
| "N/A" | -1 | "some one" |
And wait 3 seconds
# insert without default prop, failed
When executing query:
"""
INSERT VERTEX t() VALUES "1":()
"""
Then a ExecutionError should be raised at runtime: Storage Error: The not null field doesn't have a default value.
# test alter edge with default value
When executing query:
"""
Expand Down Expand Up @@ -569,14 +562,7 @@ Feature: Insert string vid of vertex and edge
"""
ALTER EDGE e CHANGE (description string NOT NULL)
"""
Then the execution should be successful
And wait 3 seconds
# insert without default prop, failed
When executing query:
"""
INSERT EDGE e() VALUES "1"->"2":()
"""
Then a SemanticError should be raised at runtime: The property `description' is not nullable and has no default value.
Then a SemanticError should be raised at runtime: Column `description' must have a default value if it's not nullable
# test alter edge with timestamp default
When executing query:
"""
Expand Down Expand Up @@ -850,6 +836,78 @@ Feature: Insert string vid of vertex and edge
DROP SPACE issue2009;
"""
Then the execution should be successful
Then drop the used space

Scenario: alter a tag to add an column which doesn't have a default value to not nullable
Given an empty graph
And create a space with following options:
| partition_num | 1 |
| replica_factor | 1 |
| vid_type | FIXED_STRING(20) |
When executing query:
"""
CREATE TAG person(age int);
"""
Then the execution should be successful
When executing query:
"""
CREATE TAG INDEX person_age_index ON person(age);
"""
Then the execution should be successful
And wait 3 seconds
When executing query:
"""
INSERT VERTEX person values "1":(23);
"""
Then the execution should be successful
When executing query:
"""
LOOKUP ON person YIELD properties(VERTEX) AS props;
"""
Then the result should be, in any order, with relax comparison:
| props |
| {age: 23} |
When executing query:
"""
ALTER TAG person ADD (gender bool NOT NULL);
"""
Then a SemanticError should be raised at runtime: Column `gender' must have a default value if it's not nullable
jievince marked this conversation as resolved.
Show resolved Hide resolved

Scenario: alter a edge to change an column which doesn't have a default value to not nullable
Given an empty graph
And create a space with following options:
| partition_num | 1 |
| replica_factor | 1 |
| vid_type | FIXED_STRING(20) |
When executing query:
"""
CREATE EDGE person(age int);
"""
Then the execution should be successful
When executing query:
"""
CREATE EDGE INDEX person_age_index ON person(age);
"""
Then the execution should be successful
And wait 3 seconds
When executing query:
"""
INSERT EDGE person values "1"->"2":(23);
"""
Then the execution should be successful
When executing query:
"""
LOOKUP ON person YIELD properties(EDGE) AS props;
"""
Then the result should be, in any order, with relax comparison:
| props |
| {age: 23} |
When executing query:
"""
ALTER EDGE person ADD (gender bool NOT NULL);
"""
Then a SemanticError should be raised at runtime: Column `gender' must have a default value if it's not nullable
jievince marked this conversation as resolved.
Show resolved Hide resolved
Then drop the used space

Scenario: Don't allow DOT in schema name
Given an empty graph
Expand All @@ -862,3 +920,4 @@ Feature: Insert string vid of vertex and edge
CREATE TAG `tag.prop`()
"""
Then a SyntaxError should be raised at runtime: Don't allow DOT in label: near `.prop`()'
Then drop the used space