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

support Nullable types #57

Open
jinmingjian opened this issue May 7, 2021 · 9 comments
Open

support Nullable types #57

jinmingjian opened this issue May 7, 2021 · 9 comments

Comments

@jinmingjian
Copy link
Contributor

part of #18

@Enochack
Copy link

/assign

@Enochack
Copy link

I'd like to pick this issue. Is it resolved now?

@jinmingjian
Copy link
Contributor Author

@Thysinner thanks. No one resolve this. And this issue is a little non-trivial. 🙏

@Enochack
Copy link

What's your idea about the issue? It will be great if I could continue to make progress on the basis of your ideas.

@jinmingjian
Copy link
Contributor Author

@Thysinner I mean you may pick up issues by your understanding. There are many good-first-issues or help-wanted issues. However, if you understand this issue, it is still great to make it done. I will help you if you meet problem.

@Enochack
Copy link

Refer to https://clickhouse.tech/docs/en/sql-reference/data-types/nullable/, the Nullable wrapped data types are restricted. And I saw the TB grammar file has restricted the wrapped data types to simple type and decimal type. So could we assume that the Nullable wrapped data types in the meta data are always valid since the grammar file is forcing users to create valid Nullable columns?

@Enochack
Copy link

Otherwise, we will always need to check the wrapped data type like this

BqlType::Nullable(bt) => match bt {
    BqlType::Int(_)
    | BqlType::UInt(_)
    | BqlType::Float(_)
    | BqlType::DateTime
    | BqlType::Date
    | BqlType::Decimal(_, _) => bt.size(),
    _ => Err(MetaError::InvalidNullableDataType),
}

Which is ugly and likely to consume more CPU

@jinmingjian
Copy link
Contributor Author

jinmingjian commented May 24, 2021

@Thysinner this is not important for prototyping. Just show a primary working PR. Then we can start from that.

@Enochack
Copy link

Thanks for the suggestion

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

No branches or pull requests

2 participants