Sounds good. I agree a feature branch is the right thing to do in these cases.
Cheers, Gidon. On Wed, Sep 26, 2018 at 2:42 PM Zoltan Ivanfi <[email protected]> wrote: > Hi, > > If the encryption code release in parquet-cpp is unused at this moment then > I think we are fine. It means that we are still free to decide any way > about the data structures without the risk of incompatility issues. In this > case I would suggest to proceed as we planned at the Parquet sync. > > Thanks, > > Zoltan > > On Wed, Sep 26, 2018 at 7:49 AM Gidon Gershinsky <[email protected]> wrote: > > > That's correct. A layer that runs the crypto classes to encrypt pages and > > structures is not merged yet. Even if the parquet.thrift is out, there is > > virtually no chance encrypted files are created with it, and certainly > not > > in production. > > > > Given a (hopefully mild) headache this last-minute feature/requirement > has > > caused, let me add details on where it is coming from. I received the > > requirement from a company that got an internal pushback from teams > working > > with unencrypted columns only, and unwilling to update Parquet libs. They > > need support for a transition period for the current readers. > > > > We have an option to say no, and stick to the original goal list. But I > > think the requirement makes sense, even if coming very late - and will > > significantly ease adoption of the Parquet encryption. Also, it turns out > > to be easy to implement, with minor changes in the code. > > > > In any case, its certainly a good idea to keep a single copy of > > parquet.thrift in the format repo. > > > > Cheers, Gidon > > > > On Wed, Sep 26, 2018 at 12:55 AM Wes McKinney <[email protected]> > wrote: > > > > > AFAIK encryption is not fully supported in parquet-cpp 1.5.0 so > > > reading and writing files with encryption is not possible. Am I wrong > > > about that? > > > > > > I'm wholly supportive of changing the project to use a released > > > version of the Parquet format, so let's do that ASAP. > > > On Tue, Sep 25, 2018 at 1:30 PM Gidon Gershinsky <[email protected]> > > wrote: > > > > > > > > Yep! (sent in parallel :) > > > > > > > > On Tue, Sep 25, 2018 at 8:19 PM Zoltan Ivanfi > <[email protected] > > > > > > > wrote: > > > > > > > > > Hi, > > > > > > > > > > As a short update, I just checked the PR for PARQUET-1419 and > > although > > > in > > > > > its current form it is a breaking change, it can be easily > rewritten > > to > > > > > become backwards-compatible so this part of the problem does not > > apply > > > any > > > > > more. > > > > > > > > > > Br, > > > > > > > > > > Zoltan > > > > > > > > > > On Tue, Sep 25, 2018 at 7:10 PM Zoltan Ivanfi <[email protected]> > > wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > On the Parquet sync we discussed that the practice of > maintaining a > > > copy > > > > > > of parquet.thrift in parquet-cpp is dangerous and that we must > take > > > care > > > > > to > > > > > > not release parquet-format changes in parquet-cpp before we > > > officially > > > > > > release them in parquet-format. As I got back to my computer and > > > started > > > > > to > > > > > > create a JIRA about this, I noticed that unfortunately this has > > > already > > > > > > happened. > > > > > > > > > > > > The encryption-releated parquet.thrift changes have not only been > > > added > > > > > to > > > > > > only parquet-format, but to parquet-cpp as well, and these > changes > > > got > > > > > > released in parquet-cpp 1.5.0. This is very unfortunate, because > > > > > > PARQUET-1419 would change the encryption in a breaking way, which > > is > > > only > > > > > > acceptable as long as the original is not released. Additionally, > > it > > > has > > > > > > been discussed that a formal voting should take place before > > > > > incorporating > > > > > > the encryption features in the format. > > > > > > > > > > > > Now that parquet-cpp has already shipped these changes, we must > > > choose > > > > > the > > > > > > lesser evil of the following two options: > > > > > > > > > > > > - Release a parquet-cpp 1.6.0 with a breaking change and risk > > that > > > > > > encrypted data files already written with parquet-cpp 1.5.0 > will > > > not > > > > > be > > > > > > readable any more. > > > > > > - Release the encryption in parquet-format as it is, > regardless > > of > > > > > > voting results and discard PARQUET-1419. > > > > > > > > > > > > Personally I have a hard time deciding which one I consider > lesser > > > evil. > > > > > > What are your opinions? > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Zoltan > > > > > > > > > > > > > > > > >
