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

tb does not support not null and default column value when creating table #201

Open
ygf11 opened this issue Jul 18, 2021 · 5 comments
Open

Comments

@ygf11
Copy link
Contributor

ygf11 commented Jul 18, 2021

when I create table with not null or default column value , server reports error:

  1. sql: CREATE TABLE person(  person_idUInt64,  nick_name String DEFAULT '123') ENGINE = BaseStorage PARTITION BY person_id
  2. 08:37:48 [DEBUG] (11) server: Found error: Unsupported language feature found

I found it does not support this now. Supporting not null syntax is easy, we can add not null matching in Rule::column_constraint, like:

 Rule::column_constraint => {
                let col = self
                    .tab
                    .columns
                    .last_mut()
                    .ok_or(LangError::CreateTableParsingError)?;
                let constr = pair.as_str().trim().to_ascii_uppercase();
                match constr.as_str() {
                    "PRIMARY KEY" => col.1.is_primary_key = true, 
                    "NOT NULL" => col.1.is_nullable = false,
                    _ => 
                        return Err(LangError::UnsupportedLangFeatureError),
                };
            }

but it need more works to support default value.

@jinmingjian
Copy link
Contributor

@ygf11 Yes, for the compatibility, it may be nice to accept a PR. Commonly, CH uses Nullable type to indicate NULL-like, others is NOT NULL in default. (Mixed Nullable and NULL keyword seems a little bad...but from mysql side, this is still good to have)

Default keyword support is great. It is asking a server side processing mechanism which we have done for partition key expressions. You may have a try.

@ygf11
Copy link
Contributor Author

ygf11 commented Jul 18, 2021

@jinmingjian ok, let me have a try.

@ygf11
Copy link
Contributor Author

ygf11 commented Aug 1, 2021

@ygf11 really appreciated for your contribution at the side of default expression of column!

This is a relative large change. I give out some basic ideas from my first eye:

  1. for large change, it is better to add some notes for better reviewers' understandings if possible:)
  2. you change the ser/des of ColumnInfo. You introduce a relatively long algorithm for ser/des, I suggest for this kind of changes, it is better to add a detail layout for ser format for bug hunting and future maintenance. Finally, this may change the latency of a query. I may do a check for this but meet the follow
  3. the new ser/des of ColumnInfo aglo breaks the old existed meta store. The better way is to add new kv into meta store tree, like shown in partition_keys_expr handling. And this is designed for extension and performance. (But sorry, I have not pointed out this in the document or comment.)
  4. the multiple-columns-expression-support consideration is good. But your codes does not solve this problem. And this reserved seems lead to the wired complexity into your codes in command_insert_into_gen_block which is hard to review.
  5. it could be good to suggest a change plan or RFC in the corresponding issue if your change is large or you feel unsure. Of course, this is not mandatory.
  6. there are some confused exceptions.
  7. command_insert_into_gen_block is the path for text-based SQL data insertions which is not permanent and not the default data ingestion of clients and drivers (in fact it is only recommend for testing).

@jinmingjian thanks for your suggestions. you are right, It is really a large pr, so changing plan or RFC is needed.
About your suggestions:

  1. Changing ColumnInfo ser/des was a helpless choice, because default expression meta info was String type, so zero copy for columnInfo struct is not working, this really need carefully design. I agree with the idea of saving in kv first for compatibility.
  2. for command_insert_into_gen_block, I found this is working in inner driver. And we can make ClickHouse-client working next.
  3. multiple-columns-expression-support is early, and it reallymake code confuse.

my plan:

  1. saving columnInfo in kv store.
  2. we should make some bql type like UInt(x) working in inner driver(for testing).
  3. we should make some bel type working with ClickHouse-client.
  4. make other bql type working like date and dateTime.
  5. other features, literal for string, multiple-clown-expression-support....

@jinmingjian
Copy link
Contributor

@ygf11 thanks for your ideas.

Changing ColumnInfo ser/des was a helpless choice

You can use serde crate, rather than your home brown algo unless yours are really faster and simpler like old zero-copy. If you really do not want to use serde, it is better to leave the document/comment about your own ser/des.

for command_insert_into_gen_block, I found this is working in inner driver. And we can make ClickHouse-client working next.

This is OK. I mean, it is better to firstly solve the problem for production-use. But this depends on your idea.

I suggest you complete this issue in a gradual way because you are in your first contribution.

For default expression, we can first implement the literal, then the general one column and then multiple columns. So, it is not necessary for involving jit side thing firstly.

Carefully design is great but it is better to leave ideas with all guys known. And sorry, the original codes have less comment. This is not good for newcomers. I will change this for further iteration. And it is greatly welcome more comments from more friends.

@ygf11
Copy link
Contributor Author

ygf11 commented Aug 2, 2021

@jinmingjian thanks for your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants