Skip to content

Support column comments#43

Open
BenoitRanque wants to merge 10 commits intomainfrom
support-column-comments
Open

Support column comments#43
BenoitRanque wants to merge 10 commits intomainfrom
support-column-comments

Conversation

@BenoitRanque
Copy link
Copy Markdown
Contributor

This PR adds support for column comments

This required a change in configuration files, because we used to store column defintions as a Map<Name, DataType>

We add a new enum so we can store both data type and optional comment. This a not a breaking change because we still support parsing the older, shorthand format.

When introspecting, we still generate the shorthand format if the comment is empty.

Additionally, we derive deserialize and serialize for ClickHouseDataType, so we can include it directly in configuration file types.

However this introduces a problem: when unable to parse a datatype, the serde json error is very unhelpful.

#[serde(untagged)]
pub enum ColumnDefinition {
    ShortHand(ClickhouseDataType),
    LongForm {
        data_type: ClickhouseDataType,
        comment: Option<String>,
    },
}

The issue is that if serde is unable to parse the ClickHouseDataType, it reports it was unable to match any of the untagged variants. So the error is very unhelpful to users.

The solution is the MaybeClickHouseDataType enum:

#[serde(untagged)]
pub enum MaybeClickhouseDataType {
    Valid(ClickHouseDataType),
    Invalid(String),
}

The Invalid variant is the result of being unable to parse the datatype. We can then handle this gracefully and tell the user exactly what the problem is, and where.

@BenoitRanque BenoitRanque force-pushed the support-column-comments branch from caf9e7a to ba2b781 Compare February 14, 2025 02:05
Copy link
Copy Markdown
Collaborator

@danieljharvey danieljharvey left a comment

Choose a reason for hiding this comment

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

🎸

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

Successfully merging this pull request may close these issues.

2 participants