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

feat: Implement ser/de for the remaining all #58

Merged
merged 12 commits into from
Sep 5, 2024

Conversation

XuQianJin-Stars
Copy link
Contributor

No description provided.

type Err = Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let parts: Vec<&str> = s.split_whitespace().collect();
Copy link
Member

Choose a reason for hiding this comment

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

Hi, every collect here involves an allocation. It would be better if we could avoid it.

We can use find instead of split and collect, for example:

impl FromStr for BinaryType {
    type Err = Error;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        if !s.starts_with(serde_utils::BINARY::NAME) {
            return DataTypeInvalidSnafu {
                message: xxx
            }
            .fail();
        }

        let Some(open_bracket) = s.find('(') else {
             return DataTypeInvalidSnafu {
                message: xxx
            }
            .fail();
        };
        let Some(close_bracket) = s.find(')') else {
             return DataTypeInvalidSnafu {
                message: xxx
            }
            .fail();
        };

        if open_bracket >= close_bracket {
            return DataTypeInvalidSnafu {
                message: xxx
            }
            .fail();
        }

        let length_str = &s[open_bracket + 1..close_bracket];
        let length = length_str.parse::<usize>().map_err(|_| DataTypeInvalidSnafu {
            message: xxx
        })?;

        let nullable = !s[close_bracket..].contains("NOT NULL");

        Ok(BinaryType {
            nullable,
            length,
        })
    }
}

type Err = Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let parts: Vec<&str> = s.split_whitespace().collect();
Copy link
Member

Choose a reason for hiding this comment

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

The same.

}
.fail();
};
let Some(close_bracket) = s.find(')') else {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about extract a method to extract the (number) format ? There are almost 8 same usage

type Err = Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
if !s.starts_with(serde_utils::TIME::NAME) || s.contains("STAMP") {
Copy link
Contributor

Choose a reason for hiding this comment

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

emm.. I feel here is a bit tricky.
BTW, Can highly_complex_nested_row_type.json pass this check ? We should also include this in tests ?

@XuQianJin-Stars
Copy link
Contributor Author

The comments on this pull request have been addressed.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution from @XuQianJin-Stars and @Aitozi's review!

Some implementation details, like the precision_scale_str.split(',').collect::<Vec<&str>>(), could be improved, but I don't think it's a blocker for this PR.

Copy link
Contributor

@Aitozi Aitozi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @XuQianJin-Stars for your updates

@Aitozi Aitozi merged commit f727960 into apache:main Sep 5, 2024
7 checks passed
@XuQianJin-Stars XuQianJin-Stars deleted the deserialize-type branch September 6, 2024 10:46
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.

3 participants