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

Variant data type support #170

Merged
merged 11 commits into from
Nov 21, 2024
Merged

Variant data type support #170

merged 11 commits into from
Nov 21, 2024

Conversation

slvrtrn
Copy link
Contributor

@slvrtrn slvrtrn commented Oct 31, 2024

Summary

Adds support for the Variant data type. Closes #143.

To discuss:

  • can we make a convenience macro to generate the enum? Or is it more of the ch2rs responsibility in this case?

Checklist

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided so that we can include it in CHANGELOG later
  • For significant changes, documentation in README and https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@serprex serprex requested review from loyd and serprex October 31, 2024 19:28
serprex
serprex previously approved these changes Oct 31, 2024
@mshustov mshustov requested a review from serprex November 5, 2024 13:43
serprex
serprex previously approved these changes Nov 8, 2024
loyd
loyd previously approved these changes Nov 19, 2024
README.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
tests/it/variant.rs Outdated Show resolved Hide resolved
tests/it/variant.rs Outdated Show resolved Hide resolved
) -> Result<()> {
panic!("newtype variant types are unsupported: `{name}::{variant}`");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: this code implicitly allows using enums at the top level. Usually, it's avoided by using either the parameter stored in the serializer struct or dedicated SerializeStruct/etc associated types.

In general, it's okay (the user will get some error anyway), but instead of more descriptive panic, it ends with "not enough data."

Also, it produces an unclear message for forgotten serde_repr (Enum8 and Enum16).

It's not a blocker for merge because it's an error anyway. But, probably, we should leave "TODO" at least in the code about it.

type Error = Error;

fn unit_variant(self) -> Result<()> {
Ok(())
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to be an error on the definition side (SomeEnum::A without any payload), innit?

We should return an error here to avoid the generic "not enough data" error. Probably, it's time to introduce Error::Unsupported instead of panics, but up to you here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Error::Unsupported and used it in this case. The rest of the panics can be changed as a follow-up.

src/rowbinary/de.rs Outdated Show resolved Hide resolved
src/rowbinary/de.rs Outdated Show resolved Hide resolved
@slvrtrn slvrtrn dismissed stale reviews from loyd and serprex via 877c5b6 November 21, 2024 15:56
@slvrtrn
Copy link
Contributor Author

slvrtrn commented Nov 21, 2024

@serprex @loyd, I updated the PR, addressing the feedback. Do you think we can merge it now?

@mshustov mshustov requested review from serprex and loyd November 21, 2024 16:49
@slvrtrn slvrtrn merged commit 9951588 into main Nov 21, 2024
5 of 6 checks passed
@mshustov mshustov deleted the variant-data-type branch November 21, 2024 16:56
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.

Support Variant(T1, T2, ...) data type
4 participants