You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@graphar.apache.org by "acezen (via GitHub)" <gi...@apache.org> on 2024/05/08 12:14:07 UTC

[PR] feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

acezen opened a new pull request, #475:
URL: https://github.com/apache/incubator-graphar/pull/475

   <!--
   Thanks for contributing to GraphAr.
   If this is your first pull request you can find detailed information on [CONTRIBUTING.md](https://github.com/apache/graphar/blob/main/CONTRIBUTING.md)
   
   The Apache GraphAr (incubating) community has restrictions on the naming of pr title. You can find instructions in
   [CONTRIBUTING.md](https://github.com/apache/graphar/blob/main/CONTRIBUTING.md#title) too.
   -->
   
   ### Reason for this PR
   <!-- 
   Why are you proposing this change? If this is already tracked in an issue, please link to the issue here.
   Explaining clearly why this change is beneficial is important for the reviewers to understand the context.
   -->
   as issue #73 describes, currently the C++ library, Java, and scala library implement the format independently and they may have some miss-match between the implementation. It's better to standardize the format with protobuf and the libraries rely on the definition.
   
   The standardized format definition would bring much benefits:
   - More clear definition to the C++/Java code
   - Make the format cross-language compatibility
   - It would  not only carry the schema of graph, but also contain the optional metadata like vertex num/edge num, the chunk statistics. ( which we serialize to files directly)
   - It can be evolve without change the library code immediately
   - [arrow](https://github.com/apache/arrow/blob/main/format/Schema.fbs) and [parquet](https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift) are define their format with IDL too.
   
   But the changes may break the breaking changes to current public APIs if we adapt the implementation to the format protocol.
   
   ### What changes are included in this PR?
   <!-- 
   There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
   -->
   
   -  Add format  protocol definitions files with google protocol buffers
   - README about the format  protocol definitions
   
   ### Are these changes tested?
   <!--
   We typically require tests for all PRs in order to:
   1. Prevent the code from being accidentally broken by subsequent changes
   2. Serve as another way to document the expected behavior of the code
   
   If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
   -->
   yes
   
   ### Are there any user-facing changes?
   <!--
   If there are user-facing changes then we may require documentation to be updated before approving the PR.
   -->
   not yet
   
   <!--
   If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
   -->
   <!-- **BREAKING CHANGE: <description>** -->
   not yet
   
   <!--
   Please uncomment the line below (and provide explanation) if the changes fix either
   (a) a security vulnerability,
   (b) a bug that caused incorrect or invalid data to be produced, or
   (c) a bug that causes a crash (even when the API contract is upheld). 
   We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
   -->
   <!-- **Critical Fix: <description>** -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2100630008

   Based on my experience with Apache Spark itself, protobuf-way may be a painful story:
   - you need to incorporate it into CI;
   - the generated code is huge (for java it may be thousands of LOC);
   - the generated code is unreadable;
   - debugging is very hard;
   - we need to decide, are we going to store the generated code in git or not (there are pros and cons);
   
   Btw, my first question is, are we able to use `buf`? (https://buf.build/docs/introduction)? Apache Spark itself uses buf because it significantly simplifies the process. I guess there shouldn't be a conflict with ASF rules. If we can, I would like to recommend to create `buf.work.yaml` and `buf.gen.yaml`. You may check examples in Apache Spark (https://github.com/apache/spark/tree/master/connector/connect/common/src/main), or I can do it because I had some experience with it;
   
   My second question, are we going to store generated code in git or not? Apache Spark uses a mixed approach, when they generate JVM-classes on the fly (via protobuf maven plugin) but store generated py files in git. For python they have a cool solution: https://github.com/apache/spark/blob/master/dev/connect-gen-protos.sh, an auto formatter of generated code, that allows to have a readable diff in git PRs;
   
   Based on my experience, Data Science / Network Analysis people and Data Engineers, who are our target auditory, are not familiar with protobuf, so we should also extends the documentation itself.
   
   P.S. It is nice that I was stuck in my work and did not write a lot of code for a new Python API. Because otherwise I would need to rewrite it again with proro :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2134688723

   > May we add `buf` as a building system? It is under Apache 2.0 and actually we need only binaries. https://github.com/bufbuild/buf
   
   Good idea, I will give a try.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2100819417

   > Based on my experience with Apache Spark itself, protobuf-way may be a painful story:
   > 
   > * you need to incorporate it into CI;
   > * the generated code is huge (for java it may be thousands of LOC);
   > * the generated code is unreadable;
   > * debugging is very hard;
   > * we need to decide, are we going to store the generated code in git or not (there are pros and cons);
   > 
   > Btw, my first question is, are we able to use `buf`? (https://buf.build/docs/introduction)? Apache Spark itself uses buf because it significantly simplifies the process. I guess there shouldn't be a conflict with ASF rules. If we can, I would like to recommend to create `buf.work.yaml` and `buf.gen.yaml`. You may check examples in Apache Spark (https://github.com/apache/spark/tree/master/connector/connect/common/src/main), or I can do it because I had some experience with it;
   > 
   > My second question, are we going to store generated code in git or not? Apache Spark uses a mixed approach, when they generate JVM-classes on the fly (via protobuf maven plugin) but store generated py files in git. For python they have a cool solution: https://github.com/apache/spark/blob/master/dev/connect-gen-protos.sh, an auto formatter of generated code, that allows to have a readable diff in git PRs;
   > 
   > Based on my experience, Data Science / Network Analysis people and Data Engineers, who are our target auditory, are not familiar with protobuf, so we should also extends the documentation itself.
   > 
   > P.S. It is nice that I was stuck in my work and did not write a lot of code for a new Python API. Because otherwise I would need to rewrite it again with proto :)
   
   Thanks Sem, the comment and advice is very helpful and insightful, `buf` is one solution and we do need to discuss about it!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2100814166

   > @acezen Because it is a very huge change, should we discuss it first in a mailing list? Like voting. Because it is a really big change, pros and cons are known too.
   
   Hi, @SemyonSinchenko sorry for the misleading,this PR is just a example or show code for format definition. As you said, it should be and I will open a discussion about the proposal and changes (may need some time to list the pros and cons about the buf). The goal I want to achieve is we should have some strategies to standardize the format definition, easy to maintain and evolve, make it cross-language compatibility.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2151973395

   @acezen May we merge it into a separate branch? I want to try to add `buf` and some additional scripts, especially for python part.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen closed pull request #475: [POC]feat(format): Add format  protocol definitions files with google protocol buffers
URL: https://github.com/apache/incubator-graphar/pull/475


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2152181944

   The change has merge into `format-definition-dev` branch for continue development, close this PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2152001974

   > @acezen May we merge it into a separate branch? I want to try to add `buf` and some additional scripts, especially for python part.
   
   Sure, I can create a branch for the format and merge this to the branch.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2134569741

   May we add `buf` as a building system? It is under Apache 2.0 and actually we need only binaries.
   https://github.com/bufbuild/buf


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] [POC]feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "acezen (via GitHub)" <gi...@apache.org>.
acezen commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2152071778

   > @acezen May we merge it into a separate branch? I want to try to add `buf` and some additional scripts, especially for python part.
   
   Hi, Sem, I have create a `format-definition-dev` branch and merge the format definition into it. You can develop base on the branch.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org


Re: [PR] feat(format): Add format protocol definitions files with google protocol buffers [incubator-graphar]

Posted by "SemyonSinchenko (via GitHub)" <gi...@apache.org>.
SemyonSinchenko commented on PR #475:
URL: https://github.com/apache/incubator-graphar/pull/475#issuecomment-2100632309

   @acezen Because it is a very huge change, should we discuss it first in a mailing list? Like voting. Because it is a really big change, pros and cons are known too.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@graphar.apache.org
For additional commands, e-mail: commits-help@graphar.apache.org