ISSUE-5829: Support field comment. #5952

RinChanNOWWW · 2022-06-13T12:56:35Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Table comment is already supported.
Support field comment for table under the new planner.

Currently, the comment is stored in DataField:

pub struct DataField {
    name: String,
    /// default_expr is serialized representation from PlanExpression
    default_expr: Option<String>,
    #[ignore_malloc_size_of = "insignificant"]
    data_type: DataTypeImpl,
    comment: Option<String>,
}

@youngsofun suggested:

just store column comments in tableInfos in catelog . don`t need to carry it in Schema. #5939 (reply in thread)

I don't know which one is better. I think the size in storage and the retrieving efficiency is the same.

And I only implemented it under the new parser and planner, should I implement it in the old one? If so, it seems need to modify sqlparser-rs.

==================

2022-06-14 12:54 Update:

Put field comments in TableMeta:

pub struct TableMeta {
    pub schema: Arc<DataSchema>,
    pub engine: String,
    pub engine_options: BTreeMap<String, String>,
    pub options: BTreeMap<String, String>,
    pub cluster_key: Option<String>,
    // The vector of cluster keys.
    pub cluster_keys: Vec<String>,
    // The default cluster keys id.
    pub default_cluster_key_id: Option<u32>,
    pub created_on: DateTime<Utc>,
    pub updated_on: DateTime<Utc>,
    pub comment: String,
    pub field_comments: Vec<String>,

    // if used in CreateTableReq, this field MUST set to None.
    pub drop_on: Option<DateTime<Utc>>,
    pub statistics: TableStatistics,
}

Like MySQL, if comment is an empty string(''), ignore it:

Changelog

New Feature

Related Issues

Fixes #5829

vercel · 2022-06-13T12:56:40Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
databend	⬜️ Ignored (Inspect)		Jun 17, 2022 at 6:18AM (UTC)

mergify · 2022-06-13T12:57:09Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

youngsofun · 2022-06-13T13:19:28Z

for example, a schema is used not only for tables but also for results and intermedia results of query and used in the plan, block etc.
but only the user-defined tables need comments.
even for user-defined tables, comments are not related to any computing.
it is only used when desc table.

RinChanNOWWW · 2022-06-13T13:58:56Z

for example, a schema is used not only for tables but also for results and intermedia results of query and used in the plan, block etc. but the user-defined tables need comments. even for user-defined tables, comments are not related to any computing. it is only used when desc table.

I see. I will change it later.

BohuTANG · 2022-06-15T01:08:50Z

tests/suites/0_stateless/05_ddl/05_0000_ddl_create_tables_v2.sql

+
+-- create table with column comment
+-- SELECT '====CREATE TABLE WITH COLUMN COMMENT====';
+-- CREATE TABLE column_comment_test (a INT COMMENT 'comment for a', b FLOAT NULL DEFAULT 0 COMMENT 'comment for b');


Why disable the test here?

Or the test_stateless_cluster_* will fail.

We should deep dive why the cluster has errors

I don't know if it is because #4544 is not finished.

Not sure why the create table need cluster, what's the error is?

ERROR 1105 (HY000) at line 62: Code: 1022, displayText = COMMENT 'comment for a' column option is not supported, please do not specify them in the CREATE TABLE statement. ERROR 1105 (HY000) at line 63: Code: 1025, displayText = Unknown table 'column_comment_test'.

This will not happen in standalone test.

Do standalone and cluster use different parsers?

If it is because the sqls will not be parsed and executed on the same node, and set enable_planner_v2 = 1 don't take effect on every node?

And CREATE TABLE column_comment_test (a INT COMMENT 'comment for a', b FLOAT NULL DEFAULT 0 COMMENT 'comment for b') is parsed on a node using planner_v1?

@RinChanNOWWW I think the reason is that we won't enable new planner in cluster mode for now, see https://github.com/datafuselabs/databend/blob/04a6c54fb958e424d2cacdb06eec09e0360525eb/query/src/servers/mysql/mysql_interactive_worker.rs#L301-L302

It is because new planner haven't implemented distributed query yet, which we are working on recently.

To resolve the test issue, you can create a particular result file for cluster mode, just as https://github.com/datafuselabs/databend/blob/main/tests/suites/0_stateless/04_explain/04_0000_explain_cluster.result

We can skip this like ./databend-test '^0[^4]_' --mode 'cluster' --run-dir 0_stateless --skip '02_0057_function_nullif'

RinChanNOWWW · 2022-06-15T06:54:07Z

It seems that the locations of error messages are different between test_stateless_cluster_linux and test_stateless_cluster_macos...

If it is better to comment the tests at this time...

leiysky · 2022-06-15T07:21:44Z

If it is better to comment the tests at this time...

I think so. @BohuTANG What do you think of this?

BohuTANG · 2022-06-15T23:27:49Z

There are conflicts files

If it is better to comment the tests at this time...

I think so. @BohuTANG What do you think of this?

Ok.
Conflict files :) @RinChanNOWWW

RinChanNOWWW · 2022-06-16T02:36:16Z

Conflicts solved.

drmingdrmer · 2022-06-16T06:40:01Z

common/meta/app/src/schema/table.rs

@@ -183,6 +183,7 @@ pub struct TableMeta {
    pub created_on: DateTime<Utc>,
    pub updated_on: DateTime<Utc>,
    pub comment: String,
+    pub field_comments: Vec<String>,


Is field_comments a vec of exactly the same number of elements as schema?

If it is, does it mean it does not distinguish between an empty string comment and no comment at all?

Is field_comments a vec of exactly the same number of elements as schema?

Yes.

If it is, does it mean it does not distinguish between an empty string comment and no comment at all?

Empty string comment will be considered as no comment, just like MySQL.

drmingdrmer · 2022-06-16T06:42:17Z

query/src/interpreters/interpreter_table_show_create.rs

+                // compatibility: creating table in the old planner will not have `fields_comments`
+                let comment = if field_comments.len() == n_fields && !field_comments[idx].is_empty()
+                {
+                    format!(" COMMENT '{}'", &field_comments[idx])


Does the field_comments[idx] need to be escaped?

need to be escaped

What does this mean?

E.g. when there is a single quote ' in the comment

Oh, I didn't consider this. I'll take a look.

create table t (a int comment 'aaa\'bbb\'ccc'); show create table t;

MySQL's result:

+-------+--------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +-------+--------------------------------------------------------------------------------------------------------------------------------------+ | t | CREATE TABLE `t` ( `a` int DEFAULT NULL COMMENT 'aaa''bbb''ccc' ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci | +-------+--------------------------------------------------------------------------------------------------------------------------------------+

MySQL split the string.

This PR's result:

+-------+------------------------------------------------------------------+ | Table | Create Table | +-------+------------------------------------------------------------------+ | t | CREATE TABLE `t` ( `a` INT COMMENT 'aaa'bbb'ccc' ) ENGINE=FUSE | +-------+------------------------------------------------------------------+

Should we be the same with MySQL's behavior?

I think we do not need to follow MySQL style, 'aaa''bbb''ccc' is hard to read.

But we should be able to create a new table with this format SQL.

You mean create table t (a int comment 'aaa''bbb''ccc'); ? This will fail during the parse phase.

Yes, so I suggest to display as create table t (a int comment 'aaa\'bbb\'ccc');, thus we can use this SQL to create table in another instance.

So the expected show-create-table result is:

+-------+------------------------------------------------------------------+ | Table | Create Table | +-------+------------------------------------------------------------------+ | t | CREATE TABLE `t` ( `a` INT COMMENT 'aaa\'bbb\'ccc' ) ENGINE=FUSE | +-------+------------------------------------------------------------------+

right?

I'll update tomorrow.

drmingdrmer · 2022-06-16T06:47:20Z

common/proto-conv/tests/it/proto_conv.rs

@@ -114,6 +114,7 @@ fn new_table_info() -> mt::TableInfo {
            created_on: Utc.ymd(2014, 11, 28).and_hms(12, 0, 9),
            updated_on: Utc.ymd(2014, 11, 29).and_hms(12, 0, 10),
            comment: s("table_comment"),
+            field_comments: vec![],


The test has to add some field comments and check if it encodes/decodes the table meta correctly.

drmingdrmer · 2022-06-17T01:29:50Z

common/proto-conv/tests/it/proto_conv.rs

-            48, 57, 32, 85, 84, 67, 170, 1, 23, 50, 48, 49, 52, 45, 49, 49, 45, 50, 57, 32, 49, 50,
-            58, 48, 48, 58, 49, 48, 32, 85, 84, 67, 178, 1, 13, 116, 97, 98, 108, 101, 95, 99, 111,
-            109, 109, 101, 110, 116, 160, 6, 1, 160, 6, 1,
+            10, 10, 8, 5, 16, 6, 160, 6, 1, 168, 6, 1, 18, 3, 102, 111, 111, 26, 3, 98, 97, 114,


These data is for compatibility test that new program can load old data. Do not modify it.

drmingdrmer

The meta data part looks good to me.

BTW, you can increase the VER to indicate it's a new version of meta data. It is not mandatory: not changing it means it has to be compatible with all changes happened in this VER=1:
https://github.com/datafuselabs/databend/blob/400fd52134cba2cc09eacad4d6a8d18f914b8410/common/proto-conv/src/util.rs#L17

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

databend-bot added the need-review label Jun 13, 2022

mergify bot added the pr-feature this PR introduces a new feature to the codebase label Jun 13, 2022

BohuTANG requested review from leiysky and andylokandy June 13, 2022 13:37

RinChanNOWWW marked this pull request as draft June 13, 2022 14:41

RinChanNOWWW marked this pull request as ready for review June 14, 2022 06:00

BohuTANG reviewed Jun 15, 2022

View reviewed changes

BohuTANG requested a review from drmingdrmer June 16, 2022 05:22

drmingdrmer reviewed Jun 16, 2022

View reviewed changes

drmingdrmer reviewed Jun 17, 2022

View reviewed changes

drmingdrmer approved these changes Jun 17, 2022

View reviewed changes

RinChanNOWWW added 6 commits June 17, 2022 14:17

feat(common-ast): support column comment in parser

419fce9

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

Support comments for columns.

1fd5169

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

add tests for creating table with column comments.

fe9ba9a

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

Move field comments to TableMeta

3fb4f17

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

Improve unit-test cases.

658f195

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

Escape ' in the comment when displayed by show create table

2020d2a

Signed-off-by: RinChanNOWWW <hzy427@gmail.com>

BohuTANG merged commit 02a7b11 into databendlabs:main Jun 17, 2022

RinChanNOWWW deleted the ISSUE-5829-FIELD-COMMENT branch June 17, 2022 07:18

BohuTANG mentioned this pull request Jan 15, 2023

create table show comment #4925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-5829: Support field comment. #5952

ISSUE-5829: Support field comment. #5952

RinChanNOWWW commented Jun 13, 2022 •

edited

Loading

vercel bot commented Jun 13, 2022 •

edited

Loading

mergify bot commented Jun 13, 2022

youngsofun commented Jun 13, 2022 •

edited

Loading

RinChanNOWWW commented Jun 13, 2022

BohuTANG Jun 15, 2022 •

edited

Loading

RinChanNOWWW Jun 15, 2022 •

edited

Loading

BohuTANG Jun 15, 2022

RinChanNOWWW Jun 15, 2022

BohuTANG Jun 15, 2022

RinChanNOWWW Jun 15, 2022

RinChanNOWWW Jun 15, 2022 •

edited

Loading

leiysky Jun 15, 2022

sundy-li Jun 16, 2022

RinChanNOWWW commented Jun 15, 2022 •

edited

Loading

leiysky commented Jun 15, 2022

BohuTANG commented Jun 15, 2022

RinChanNOWWW commented Jun 16, 2022

drmingdrmer Jun 16, 2022

RinChanNOWWW Jun 16, 2022 •

edited

Loading

drmingdrmer Jun 16, 2022

RinChanNOWWW Jun 16, 2022

drmingdrmer Jun 16, 2022

RinChanNOWWW Jun 16, 2022

RinChanNOWWW Jun 16, 2022

sundy-li Jun 16, 2022

sundy-li Jun 16, 2022

RinChanNOWWW Jun 16, 2022 •

edited

Loading

sundy-li Jun 16, 2022

RinChanNOWWW Jun 16, 2022

drmingdrmer Jun 16, 2022

RinChanNOWWW Jun 16, 2022

drmingdrmer Jun 17, 2022

RinChanNOWWW Jun 17, 2022

drmingdrmer left a comment

ISSUE-5829: Support field comment. #5952

ISSUE-5829: Support field comment. #5952

Conversation

RinChanNOWWW commented Jun 13, 2022 • edited Loading

Summary

Changelog

Related Issues

vercel bot commented Jun 13, 2022 • edited Loading

mergify bot commented Jun 13, 2022

youngsofun commented Jun 13, 2022 • edited Loading

RinChanNOWWW commented Jun 13, 2022

BohuTANG Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

RinChanNOWWW Jun 15, 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

Choose a reason for hiding this comment

RinChanNOWWW Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RinChanNOWWW commented Jun 15, 2022 • edited Loading

leiysky commented Jun 15, 2022

BohuTANG commented Jun 15, 2022

RinChanNOWWW commented Jun 16, 2022

Choose a reason for hiding this comment

RinChanNOWWW Jun 16, 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

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

RinChanNOWWW Jun 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drmingdrmer left a comment

Choose a reason for hiding this comment

RinChanNOWWW commented Jun 13, 2022 •

edited

Loading

vercel bot commented Jun 13, 2022 •

edited

Loading

youngsofun commented Jun 13, 2022 •

edited

Loading

BohuTANG Jun 15, 2022 •

edited

Loading

RinChanNOWWW Jun 15, 2022 •

edited

Loading

RinChanNOWWW Jun 15, 2022 •

edited

Loading

RinChanNOWWW commented Jun 15, 2022 •

edited

Loading

RinChanNOWWW Jun 16, 2022 •

edited

Loading

RinChanNOWWW Jun 16, 2022 •

edited

Loading