SYaoJun commented on PR #966: URL: https://github.com/apache/incubator-graphar/pull/966#issuecomment-5571932999
> @SemyonSinchenko @SYaoJun a review here would be appreciated whenever you have time. Rebased on current `main`, no conflicts, CI green. > > This follows the same shape as the local adapter merged in #960: it stays behind the storage API and keeps GraphAr layout and format concerns out of storage. @keksmd Thanks for your contribution! I am not very familiar with the Java codebase, so it would be helpful to also get a review from Sem or Xiaokang. I also ran this PR through an AI review tool and received some suggestions. I will paste them below for reference. I have a few questions about the overall design. I may be missing some context here, but I am trying to understand the motivation behind this PR. Why does GraphAr need to add S3 storage support? I noticed that there is already a `java-info` directory containing a pure Java reader and writer. Could you explain why the new implementation is not built on top of the existing structure? Also, could you clarify the relationship between the new S3-related code and the existing `java-info` implementation? It would be very helpful if you could add a high-level architecture diagram showing the relationships between these components and the overall design rationale. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
