Restrict schema type conversion #5098

wenhaocs · 2022-12-23T02:41:23Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

https://github.com/vesoft-inc/nebula-ent/issues/1786

Description:

We now restrict the type conversion in schema change. Only the following are allowed:
(1) fixed string -> larger fixed string
(2) fixed string -> string
(3) thinner int -> wider int
(4) float -> double
(5) no type change

All other conversions are not allowed.

E.g.,
Change string to fixed string

Change int8 to int16 and then back to int8

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

src/meta/MetaServiceUtils.cpp

yixinglu · 2022-12-23T06:38:28Z

src/meta/MetaServiceUtils.cpp

+  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.


shall we forbid the type conversion from boolean to int?

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

prefer strict

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

critical27

LGTM, good job.

critical27 · 2022-12-23T08:40:14Z

It seems some tck test cases failed~

wenhaocs · 2022-12-25T01:16:55Z

src/meta/test/ProcessorTest.cpp

@@ -1947,7 +2042,7 @@ TEST(ProcessorTest, AlterEdgeTest) {
    cpp2::Schema changeSch;
    cpp2::ColumnDef column;
    column.name = "edge_0_col_10";
-    column.type.type_ref() = PropertyType::DOUBLE;
+    column.type.type_ref() = PropertyType::INT64;


Note that the current column type of col_10 is int64. The failure should come from changing col on ttl. So I keep the column type the same.

wenhaocs · 2022-12-25T01:17:00Z

src/meta/test/ProcessorTest.cpp

@@ -1947,7 +2042,7 @@ TEST(ProcessorTest, AlterEdgeTest) {
    cpp2::Schema changeSch;
    cpp2::ColumnDef column;
    column.name = "edge_0_col_10";
-    column.type.type_ref() = PropertyType::DOUBLE;
+    column.type.type_ref() = PropertyType::INT64;


Note that the current column type of col_10 is int64. The failure should come from changing col with ttl. So I keep the column type the same, to avoid the error coming from illegal column type change.

wenhaocs · 2022-12-25T01:19:21Z

src/meta/test/ProcessorTest.cpp

@@ -1452,7 +1454,7 @@ TEST(ProcessorTest, AlterTagTest) {
    cpp2::Schema changeSch;
    cpp2::ColumnDef column;
    column.name = "tag_0_col_10";
-    column.type.type_ref() = PropertyType::DOUBLE;
+    column.type.type_ref() = PropertyType::INT64;


Note that the current column type of col_10 is int64. The failure should come from changing col with ttl. So I keep the column type the same, to avoid the error coming from illegal column type change.

wenhaocs · 2022-12-25T01:22:18Z

tests/tck/features/ttl/TTL.feature

@@ -340,27 +350,27 @@ Feature: TTLTest
      | "player" | 'CREATE EDGE `player` (\n `id` int64 NULL,\n `name` string NULL,\n `age` int64 NULL,\n `address` string NULL,\n `score` float NULL\n) ttl_duration = 0, ttl_col = ""' |
    When executing query:
      """
-      ALTER EDGE player change(name int), drop(name);
+      ALTER EDGE player change(name string), drop(name);


The error should come from the intended, not from illegal type change. So here I keep type the same.

The same for the followings.

Sophie-Xie · 2022-12-26T09:01:39Z

@wenhaocs pls fix the tck~

fix tck

wenhaocs · 2022-12-28T00:08:38Z

@wenhaocs pls fix the tck~

Done!

foesa-yang · 2023-01-29T06:02:18Z

vesoft-inc/nebula-docs-cn#2516

wenhaocs requested review from critical27, pengweisong and SuperYoko December 23, 2022 03:00

wenhaocs added ready-for-testing PR: ready for the CI test ready for review labels Dec 23, 2022

yixinglu reviewed Dec 23, 2022

View reviewed changes

xtcyclist previously approved these changes Dec 23, 2022

View reviewed changes

critical27 previously approved these changes Dec 23, 2022

View reviewed changes

wenhaocs dismissed stale reviews from critical27 and xtcyclist via f4fdadd December 25, 2022 01:14

wenhaocs force-pushed the haowen/fix_schema branch from 11cbabe to f4fdadd Compare December 25, 2022 01:14

wenhaocs commented Dec 25, 2022

View reviewed changes

Sophie-Xie added the priority/hi-pri Priority: high label Dec 26, 2022

xtcyclist force-pushed the haowen/fix_schema branch from ee98b99 to f2eb3d9 Compare December 26, 2022 05:28

critical27 previously approved these changes Dec 26, 2022

View reviewed changes

xtcyclist dismissed critical27’s stale review via 6a5f611 December 26, 2022 06:18

xtcyclist force-pushed the haowen/fix_schema branch from 4dd6524 to 6a5f611 Compare December 26, 2022 06:18

wenhaocs added 9 commits December 28, 2022 04:04

add a function to check legitimacy

51fffc4

fix test

0e5be9a

change to warning level

7b44fe5

handle unknown type

c314b98

fix tck

4b529b8

change to log error

21576c2

update test to look better

bceb3d2

fix tck

lint

e5e8752

lint

18d01fb

wenhaocs added 4 commits December 28, 2022 04:04

change int32 to int64 in tck

494dfac

fix test_insert_vertex_and_edge_test_2 tck

4c25970

fix tck

f3cadba

fix tck

b12d547

wenhaocs force-pushed the haowen/fix_schema branch from 9a132be to b12d547 Compare December 27, 2022 20:04

wenhaocs added 4 commits December 28, 2022 06:04

fix tck

5370f9e

format

a9bb5ea

fix tck

7fac0a1

fix tck

62cdd6b

xtcyclist approved these changes Dec 28, 2022

View reviewed changes

critical27 approved these changes Dec 28, 2022

View reviewed changes

Merge branch 'master' into haowen/fix_schema

f914b61

Sophie-Xie merged commit f519503 into vesoft-inc:master Dec 28, 2022

wey-gu mentioned this pull request Dec 31, 2022

Weekly Report 2022-12-30 vesoft-inc/nebula-community#171

Closed

Sophie-Xie added the doc affected PR: improvements or additions to documentation label Jan 12, 2023

foesa-yang requested review from foesa-yang and removed request for foesa-yang January 17, 2023 06:32

foesa-yang self-assigned this Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict schema type conversion #5098

Restrict schema type conversion #5098

wenhaocs commented Dec 23, 2022 •

edited

Loading

yixinglu Dec 23, 2022

wenhaocs Dec 23, 2022

HarrisChu Dec 23, 2022

critical27 Dec 23, 2022

critical27 left a comment

critical27 commented Dec 23, 2022

wenhaocs Dec 25, 2022

wenhaocs Dec 25, 2022 •

edited

Loading

wenhaocs Dec 25, 2022

wenhaocs Dec 25, 2022

wenhaocs Dec 25, 2022

Sophie-Xie commented Dec 26, 2022

wenhaocs commented Dec 28, 2022

foesa-yang commented Jan 29, 2023

Restrict schema type conversion #5098

Restrict schema type conversion #5098

Conversation

wenhaocs commented Dec 23, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

Checklist:

Release notes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

critical27 left a comment

Choose a reason for hiding this comment

critical27 commented Dec 23, 2022

Choose a reason for hiding this comment

wenhaocs Dec 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sophie-Xie commented Dec 26, 2022

wenhaocs commented Dec 28, 2022

foesa-yang commented Jan 29, 2023

wenhaocs commented Dec 23, 2022 •

edited

Loading

wenhaocs Dec 25, 2022 •

edited

Loading