Hi Wes, Yep, I meant private as in not under apache/arrow. Will be more careful with that term.
https://github.com/apache/parquet-testing seems to currently contain only data (.parquet) files. Our samples are C++ and Java code files, that write and read .parquet files in different encryption configurations. We can add these encrypted .parquet files to the apache/parquet-testing repo. Let us know if we also should add the samples themselves there (by creating src folders in this repo), or add them to the apache/arrow/mr repos, or to keep them in our personal repos (the ones I've called private before). These samples are based on the standard parquet samples (such as https://github.com/apache/arrow/blob/master/cpp/examples/parquet/low-level-api/reader-writer.cc), with a loop that creates different encryption configs, and writes/reads the same data with them. Cheers, Gidon. On Tue, May 28, 2019 at 5:51 PM Wes McKinney <[email protected]> wrote: > hi Gidon, > > Surely you don't mean "private" private -- this was always on GitHub > right (otherwise this runs against the Apache Way)? > > I think we should try to have all of the testing code in the upstream > repositories. If there are sample files you need to check in (and they > are not too large), please use the apache/parquet-testing repository > > - Wes > > On Tue, May 28, 2019 at 8:00 AM Gidon Gershinsky <[email protected]> wrote: > > > > Hi Deepak, > > > > Sounds good. Here goes the current cpp interop sample (not final yet). We > > have a sample with a similar code in Java. > > > https://github.com/newrevit13/arrow/blob/interop_tests/cpp/examples/parquet/low-level-api/encryption-interop-tests.cc > > > > > > Please let me know if we should add this file to the cpp PR and its > staging > > branch (under parquet/low-level-api, or another folder?) > > - or leave this file in our private branch. > > > > Cheers, Gidon. > > > > On Tue, May 28, 2019 at 7:25 AM Deepak Majeti <[email protected]> > > wrote: > > > > > Hi Gidon, > > > > > > I agree with your point 2). I merged the PR for OpenSSL build support. > We > > > can do the same for PR-3520 after you push the changes this week. We > can > > > then simply rebase PR-2555 and make the final review. > > > For 1) I would like to take a look at the inter-op testing framework > first. > > > Can you share the inter-op sample? Thanks. > > > > > > > > > On Mon, May 27, 2019 at 7:21 PM Gidon Gershinsky <[email protected]> > wrote: > > > > > > > Hi Deepak, > > > > > > > > If it simplifies the code review, we can make this change. Actually, > > > we're > > > > already creating a single branch that has the parquet-cpp with > encryption > > > > in one place, ready for build and sample run. This is a private > branch - > > > > but we can do the same for the pull request (2555), making it the > staging > > > > branch. However, there are a number of technical details we need to > > > address > > > > first: > > > > > > > > 1) The private branch we're creating contains not only the cpp > encryption > > > > sample, but also an additional interop sample that checks > > > interoperability > > > > with parquet-mr encryption. > > > > Do we want to add this interop sample to the staging branch? Should > this > > > > sample be merged in arrow/parquet-cpp for future interop testing with > > > Java > > > > encryption? Or this sample can stay in the staging branch for the > > > duration > > > > of the review process, and be removed just before the staging PR is > > > merged > > > > in arrow/parquet-cpp? Or we dont add this interop sample to the > staging > > > > branch at all, and keep our private branch for interop testing? Let > us > > > know > > > > your preference. > > > > > > > > 2) PR 3520 has been reviewed. We'll make the requested changes this > week, > > > > and update the PR. It adds 2 new files, and doesnt touch the existing > > > ones > > > > - if you guys will be ok with the updated version, you can merge it, > and > > > > then the staging PR (2555) will automatically pick the 3520 patch > upon > > > > rebasing to the then-current master state. But, if further changes > will > > > be > > > > required in the #3520 files, we'll add them to the staging PR. > > > > > > > > Cheers, Gidon. > > > > > > > > On Fri, May 24, 2019 at 9:02 PM Deepak Majeti < > [email protected]> > > > > wrote: > > > > > > > > > Hi Gidon and Tham, > > > > > > > > > > I opened a PR for OpenSSL build support here: > > > > > https://github.com/apache/arrow/pull/4384/files > > > > > However, your PRs need to be rebased/merged with master. > > > > > Why don't we keep one staging PR for C++? Probably make > > > > > https://github.com/apache/arrow/pull/2555 the staging PR and > > > cherry-pick > > > > > https://github.com/apache/arrow/pull/3520/files and the above PR > onto > > > > it. > > > > > The later PRs are tiny anyway. This simplifies code review, syncing > > > with > > > > > the master, as well as building the encryption work while > reviewing. > > > > > > > > > > > > > > > On Wed, May 22, 2019 at 12:43 PM Gidon Gershinsky < > [email protected]> > > > > > wrote: > > > > > > > > > > > Thanks guys. We'll create a doc with a description of the current > > > > interop > > > > > > tests, including details on how to build/run them. > > > > > > > > > > > > > > > > > > On Wed, May 22, 2019 at 7:22 PM Deepak Majeti < > > > [email protected] > > > > > > > > > > > wrote: > > > > > > > > > > > > > I will make the first attempt to review the C++ code. I > reviewed > > > part > > > > > of > > > > > > it > > > > > > > a while ago. > > > > > > > Java-C++ inter-operation testing is my main concern. Can you > share > > > > some > > > > > > > details on what has been tested? > > > > > > > I need to update myself with the key management part and see > if we > > > > can > > > > > > add > > > > > > > some test parquet files written in Java along with the keys to > the > > > > data > > > > > > > folder. > > > > > > > > > > > > > > On Wed, May 22, 2019 at 10:18 AM Wes McKinney < > [email protected] > > > > > > > > > > wrote: > > > > > > > > > > > > > > > hi folks, > > > > > > > > > > > > > > > > I'm extraordinarily busy but it seems likely that the code > review > > > > for > > > > > > > > Parquet C++ is largely going to fall on my shoulders. Since > this > > > > is a > > > > > > > > large patch with many touch-points in the C++ codebase, and > there > > > > > > > > hasn't been much feedback over the course of its > development, I > > > > would > > > > > > > > like to leave thoughtful code review so that we can avoid > issues > > > > that > > > > > > > > may lead to burdensome maintenance issues. > > > > > > > > > > > > > > > > On the OpenSSL issue -- there are many CMake projects in the > wild > > > > > that > > > > > > > > use OpenSSL and there are many examples of how to incorporate > > > other > > > > > > > > dependencies into the Arrow build system. I don't have time > to > > > work > > > > > on > > > > > > > > this right now so someone else will have to sort out the > > > toolchain > > > > > > > > issue > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > On Mon, May 20, 2019 at 8:48 AM Gidon Gershinsky < > > > [email protected] > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > <re-sending from another account> > > > > > > > > > > > > > > > > > > Tham, thank you for this! and for volunteering early for > the > > > C++ > > > > > > > version > > > > > > > > > work, driving it forward and creating the bulk of the > > > parquet-cpp > > > > > > > > > encryption code along the way. > > > > > > > > > > > > > > > > > > @All - this announcement means that two implementations of > > > > Parquet > > > > > > > > > encryption, fully conforming to the formal specification, > are > > > > > > available > > > > > > > > > today. Thanks to Revital for contributing to C++ version > > > > compliance > > > > > > > with > > > > > > > > > the encryption spec, and for running a set of basic > Java-C++ > > > > > > encryption > > > > > > > > > interoperability tests. We have tested plaintext and > encrypted > > > > > footer > > > > > > > > > modes, GCM and GCM_CTR algorithms, new and legacy readers. > > > Files > > > > > > > written > > > > > > > > > with parquet-cpp are successfully parsed by parquet-mr, and > > > vice > > > > > > versa. > > > > > > > > > > > > > > > > > > Let me also thank Junjie, Nandor, Anna and Xinli for their > > > > support > > > > > > and > > > > > > > > vote > > > > > > > > > for the encryption specification - along with the PMC > folks. > > > > > > > > > > > > > > > > > > All parquet-format pull requests are merged by now into the > > > > > > encryption > > > > > > > > > branch, > > > > > > > > > https://github.com/apache/parquet-format/tree/encryption > > > > > > > > > > > > > > > > > > The community is welcome to review the parquet-mr pull > > > requests, > > > > in > > > > > > the > > > > > > > > > following order: > > > > > > > > > https://github.com/apache/parquet-mr/pull/613 > > > > > > > > > https://github.com/apache/parquet-mr/pull/614 > > > > > > > > > https://github.com/apache/parquet-mr/pull/643 > > > > > > > > > > > > > > > > > > > > > > > > > > > Currently, an end-to-end implementation of Java (mr) > Parquet > > > > > > encryption > > > > > > > > is > > > > > > > > > collected in this branch: > > > > > > > > > https://github.com/ggershinsky/parquet-mr/tree/encr > > > > > > > > > Thanks to Xinli for working with this branch code, and > > > > contributing > > > > > > to > > > > > > > it > > > > > > > > > based on his field experience. Everybody is welcome to do > the > > > > same. > > > > > > > > > @All - it would be helpful to review & merge the above PRs > in > > > > > > > > > apache/parquet-mr/encryption, so that folks can work with > it > > > > > instead > > > > > > of > > > > > > > > my > > > > > > > > > private branch.. > > > > > > > > > > > > > > > > > > And I certainly second Tham's call to review & merge the > > > > > parquet-cpp > > > > > > > pull > > > > > > > > > requests. By now, we have a number of companies starting to > > > > utilize > > > > > > > > Parquet > > > > > > > > > encryption (both C++ and Java), including IBM. > > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, Gidon. > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, May 20, 2019 at 1:40 PM Tham Ha <[email protected]> > > > wrote: > > > > > > > > > > > > > > > > > > > Hi community, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > After a long time of development, I'm honor to announce > that > > > we > > > > > > have > > > > > > > > just > > > > > > > > > > completed C++ parquet encryption module which implements > > > > > encryption > > > > > > > in > > > > > > > > low > > > > > > > > > > level api and with examples included. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > To have this feature completed, I would like to thank > Gidon > > > and > > > > > > > > Revital for > > > > > > > > > > their contribution. > > > > > > > > > > > > > > > > > > > > Gidon had a key role in encryption design and in writing > Java > > > > > > version > > > > > > > > code > > > > > > > > > > on which we based on to write C++ version. He also wrote > > > crypto > > > > > > > > package in > > > > > > > > > > C++ version. > > > > > > > > > > > > > > > > > > > > Revital and me has been joining together in writing C++ > > > > version. > > > > > > > > Revital > > > > > > > > > > was responsible for AAD calculations, API updating (to > be the > > > > > same > > > > > > > with > > > > > > > > > > Java version) and Java-C++ inter-operation testing. I was > > > > writing > > > > > > the > > > > > > > > first > > > > > > > > > > draft (properties, metadata, writer, reader) and keeping > them > > > > > > updated > > > > > > > > when > > > > > > > > > > crypto package change. > > > > > > > > > > > > > > > > > > > > We have had a great time to cooperate. Thank Gidon and > > > Revital > > > > > for > > > > > > > all > > > > > > > > > > guide and experience I have received from them, too. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Here are the links of pull requests: > > > > > > > > > > > > > > > > > > > > 1) encryption module (properties, metadata, writer, > reader): > > > > > > > > > > https://github.com/apache/arrow/pull/2555. > > > > > > > > > > > > > > > > > > > > 2) some merged pull requests for new thrift structure and > > > > crypto > > > > > > > > algorithm, > > > > > > > > > > and one still open: > > > https://github.com/apache/arrow/pull/3520 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > However, in order to make (1) buildable with current > build > > > > > scripts, > > > > > > > we > > > > > > > > need > > > > > > > > > > “adding openssl in C++ build toolchain” which is > mentioned in > > > > > this > > > > > > > > jira: > > > > > > > > > > https://issues.apache.org/jira/browse/ARROW-4302. I > will be > > > > > > grateful > > > > > > > > if > > > > > > > > > > someone could help fullfill this work. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > About current pull requests, they has been currently > using in > > > > our > > > > > > > > > > development phase at Emotiv (https://www.emotiv.com/). > We > > > love > > > > > > using > > > > > > > > > > parquet files to store EEG data. We are going to release > a > > > > > product > > > > > > > with > > > > > > > > > > encrypted parquet files soon and look forward to the > official > > > > > > release > > > > > > > > of > > > > > > > > > > parquet encryption feature. So it will be many thank and > > > great > > > > > > honor > > > > > > > to > > > > > > > > > > have you review and merge them (if qualified). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thank you very much! > > > > > > > > > > > > > > > > > > > > Tham > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > regards, > > > > > > > Deepak Majeti > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > regards, > > > > > Deepak Majeti > > > > > > > > > > > > > > > > > > -- > > > regards, > > > Deepak Majeti > > > >
