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

[C++][Improvement] Add validation of different levels for builders in C++ library #181

Merged
merged 3 commits into from
Jun 2, 2023

Conversation

lixueclaire
Copy link
Contributor

Proposed changes

Similar to the GraphAr C++ SDK writers, we could add the implementation of various validation levels in the builders (VerticesBuilder & EdgesBuilder). A strong validation level could ensure the consistency of data types between the data and meta info before being written.

Types of changes

What types of changes does your code introduce to GraphAr?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

solve issue #171

@github-actions
Copy link

github-actions bot commented May 31, 2023

🎊 PR Preview 063e2e1 has been successfully built and deployed to https://alibaba-graphar-build-pr-181.surge.sh

🤖 By surge-preview

@lixueclaire lixueclaire changed the title [WIP][C++][Improvement] Add validation of different levels for builders in C++ library [C++][Improvement] Add validation of different levels for builders in C++ library Jun 1, 2023
@lixueclaire lixueclaire requested a review from acezen June 1, 2023 03:53
Copy link
Contributor

@acezen acezen left a comment

Choose a reason for hiding this comment

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

About the validation level of writer or builder, I have some idea, it may not be right, but we can discuss together.

  1. As my understanding , beside the global validation level for builder and writer, you also provide a fine-grained validation that every adding vertex/edge/table could have a validation level itself. The fine-grained validation is a little complicated to me. In my opinion, the validation for the same builder or writer better to be consistent.
    A similar example is the validation level of MongoDB

  2. The expose of the Validate method which means that the object is not just a Builder or Writer, but also a Validator to user. Do we want to give the context that Builder is also a Validator?

}
Status Validate(
const Vertex& v, IdType index = -1,
ValidateLevel validate_level = ValidateLevel::default_validate) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the changes. I have some question about the validate level here:

  1. What's the difference between this validate and builder's validate?
    if they are the same, why the default value is different?
  2. it seems that the method could set different validate level for different vertex, but it seems a little weird different vertices have different validate level in the same builder?

Copy link
Contributor Author

@lixueclaire lixueclaire Jun 1, 2023

Choose a reason for hiding this comment

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

default_validate is to use the validate_level of the builder, which is a global setting and will be passed to construct the low-level writer. On the other hand, we also support to specify a validate_level that is only used in the function of adding a vertex.

We provide different levels to be more flexible. For example, a group of vertices are generated together, and have the same schema. The user may only need to validate the first vertex, and others may being added safely.

@lixueclaire
Copy link
Contributor Author

lixueclaire commented Jun 1, 2023

About the validation level of writer or builder, I have some idea, it may not be right, but we can discuss together.

  1. As my understanding , beside the global validation level for builder and writer, you also provide a fine-grained validation that every adding vertex/edge/table could have a validation level itself. The fine-grained validation is a little complicated to me. In my opinion, the validation for the same builder or writer better to be consistent.
    A similar example is the validation level of MongoDB
  2. The expose of the Validate method which means that the object is not just a Builder or Writer, but also a Validator to user. Do we want to give the context that Builder is also a Validator?

We expose the method to users so they can check if an operation is valid before applying.
In my opinion, The Validator is a necessary part of Builder or Writer. It is not used for validate the data itself, but to validate an operation of Builder or Writer.

@acezen
Copy link
Contributor

acezen commented Jun 2, 2023

About the validation level of writer or builder, I have some idea, it may not be right, but we can discuss together.

  1. As my understanding , beside the global validation level for builder and writer, you also provide a fine-grained validation that every adding vertex/edge/table could have a validation level itself. The fine-grained validation is a little complicated to me. In my opinion, the validation for the same builder or writer better to be consistent.
    A similar example is the validation level of MongoDB
  2. The expose of the Validate method which means that the object is not just a Builder or Writer, but also a Validator to user. Do we want to give the context that Builder is also a Validator?

We expose the method to users so they can check if an operation is valid before applying. In my opinion, The Validator is a necessary part of Builder or Writer. It is not used for validate the data itself, but to validate an operation of Builder or Writer.

understand, that make reason to me (but i still think is a little complicated). We can merge the pull request first and discuss about this at next meeting.

Copy link
Contributor

@acezen acezen left a comment

Choose a reason for hiding this comment

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

LGTM

@lixueclaire
Copy link
Contributor Author

About the validation level of writer or builder, I have some idea, it may not be right, but we can discuss together.

  1. As my understanding , beside the global validation level for builder and writer, you also provide a fine-grained validation that every adding vertex/edge/table could have a validation level itself. The fine-grained validation is a little complicated to me. In my opinion, the validation for the same builder or writer better to be consistent.
    A similar example is the validation level of MongoDB
  2. The expose of the Validate method which means that the object is not just a Builder or Writer, but also a Validator to user. Do we want to give the context that Builder is also a Validator?

We expose the method to users so they can check if an operation is valid before applying. In my opinion, The Validator is a necessary part of Builder or Writer. It is not used for validate the data itself, but to validate an operation of Builder or Writer.

understand, that make reason to me (but i still think is a little complicated). We can merge the pull request first and discuss about this at next meeting.

I agree with you. This pull request is a step towards aligning the validation levels for Builder with Writer, since Builder is a high-level wrapper of Writer.
However, we could continue this discussion after merging this change to focus on developing a more comprehensive and consistent approach to validation across all of GraphAr's tools and libraries, not just for Builder and Writer.

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