This certainly sounds good to me. Cheers, Gidon
On Wed, Feb 17, 2021 at 7:36 PM Antoine Pitrou <anto...@python.org> wrote: > > I don't think any notion of threading should be present in the > implementation, except for the required locks around shared structures. > I don't know where the idea of a "main thread" comes from, but it > probably shouldn't exist in a C++ library. > > Regards > > Antoine. > > > > Le 17/02/2021 à 18:34, Gidon Gershinsky a écrit : > > Just to clarify. There are two options, which one do you refer to? A > design > > with a main thread that handles projections and the keys (relevant for > the > > projected columns); or the current code with any thread allowed to handle > > full file reading, inc the footer, column projections and their keys? Can > > you finalize this with Micah? > > The good news is, Tham is still interested to resume this work, and is ok > > with either option. Please let her know whether the current threading > model > > stays, or should be modified with the changes proposed in the doc (for > the > > latter, some guidance with the details would be needed). > > > > Cheers, Gidon > > > > > > On Wed, Feb 17, 2021 at 2:40 PM Antoine Pitrou <anto...@python.org> > wrote: > > > >> > >> > >> Le 17/02/2021 à 12:47, Gidon Gershinsky a écrit : > >>> From the doc, > >>> "To maintain consistency with the style of parquet-cpp, the above > >>> structures should not be explicitly synchronized with individual > mutexes. > >>> In the case of a parquet::arrow::FileReader, the request to read a > given > >>> selection of row groups and columns is issued from a single main > thread. > >>> Note that this does require that all keys required for a read are > >> assembled > >>> on the main thread so that DecryptionKeyRetriever objects are not > >> directly > >>> accessing any caches" > >>> > >>> The current PR code doesn't require a single main thread. Any thread > can > >>> read any file, both footer and pages. So the key cache is shared, to > save > >>> N-1 interactions with the KMS server. > >> > >> I don't think there's any contention on this. IMHO the only concerns > >> are about the implementation, not the semantics. > >> > >> Best regards > >> > >> Antoine. > >> > >> > >>> > >>> Cheers, Gidon > >>> > >>> > >>> On Wed, Feb 17, 2021 at 12:49 PM Antoine Pitrou <anto...@python.org> > >> wrote: > >>> > >>>> > >>>> I'm not sure a threading model is expected for an encryption layer. > Am > >>>> I missing something? > >>>> > >>>> Regards > >>>> > >>>> Antoine. > >>>> > >>>> > >>>> Le 17/02/2021 à 06:59, Gidon Gershinsky a écrit : > >>>>> Precisely, the main change is in the threading model. Afaik, the > >> document > >>>>> proposes a model that fits pandas, but might be problematic for other > >>>> users > >>>>> of this library. > >>>>> Technically, this is not showstopper though; if the community decides > >> on > >>>>> this model, it will be compatible with the high-level encryption > >> design; > >>>>> but the change implementation would need to be done by pandas experts > >>>> (not > >>>>> us; but we'll help where we can). > >>>>> Micah, you know this subject (and the community) better than we do - > >> we'd > >>>>> much appreciate it if you'd take a lead on removing this roadblock. > >>>>> > >>>>> Cheers, Gidon > >>>>> > >>>>> > >>>>> On Wed, Feb 17, 2021 at 6:08 AM Micah Kornfield < > emkornfi...@gmail.com > >>> > >>>>> wrote: > >>>>> > >>>>>> I think some of the comments might be conflicting. One of the > >> concerns > >>>>>> (that I would need to refresh myself on to offer an opinion which > was > >>>>>> covered in Ben's doc) was the threading model we expect in the > >> library. > >>>>>> > >>>>>> On Tue, Feb 16, 2021 at 8:03 AM Antoine Pitrou <anto...@python.org> > >>>> wrote: > >>>>>> > >>>>>>> > >>>>>>> Hi Gidon, > >>>>>>> > >>>>>>> Le 16/02/2021 à 16:42, Gidon Gershinsky a écrit : > >>>>>>>> Regarding the high-level layer, I think it waits for a progress at > >>>>>>>> > >>>>>>> > >>>>>> > >>>> > >> > https://docs.google.com/document/d/11qz84ajysvVo5ZAV9mXKOeh6ay4-xgkBrubggCP5220/edit?usp=sharing > >>>>>>>> No activity there since last November. This is unfortunate, > because > >>>>>> Tham > >>>>>>>> has put a lot of work in coding the high-level layer (and > addressing > >>>>>> 200+ > >>>>>>>> review comments) in the PR > >> https://github.com/apache/arrow/pull/8023. > >>>>>>> The > >>>>>>>> code is functional, compatible with the Java version in > parquet-mr, > >>>> and > >>>>>>> can > >>>>>>>> be updated with the threading changes in the doc above. I hope all > >>>> this > >>>>>>>> good work will not be wasted. > >>>>>>> > >>>>>>> I'm sorry for the possibly frustrating process. Looking at the PR, > >>>>>>> though, it seems a bunch of comments were not addressed. Is it > >>>> possible > >>>>>>> to go through them and ensure they get an answer and/or a > resolution? > >>>>>>> > >>>>>>> Best regards > >>>>>>> > >>>>>>> Antoine. > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> Cheers, Gidon > >>>>>>>> > >>>>>>>> > >>>>>>>> On Sat, Feb 13, 2021 at 6:52 AM Micah Kornfield < > >>>> emkornfi...@gmail.com > >>>>>>> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>> My thoughts: > >>>>>>>>> 1. I've lost track of the higher level encryption implementation > >> in > >>>>>>> C++. > >>>>>>>>> I think we were trying to come to a consensus on the > >> threading/thread > >>>>>>>>> safety model? > >>>>>>>>> > >>>>>>>>> 2. I'm open to exposing the lower level encryption libraries in > >>>>>> python > >>>>>>>>> (without appropriate namespacing/communication). It seems at > least > >>>>>> for > >>>>>>>>> reading, there is potentially less harm (I'll caveat that with > I'm > >>>>>> not a > >>>>>>>>> security expert). Are both the low level read and write > >>>>>> implementations > >>>>>>>>> necessary? (it probably makes sense to have a few smaller PRs > for > >>>>>>> exposing > >>>>>>>>> this functionality anyways). > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Wed, Feb 10, 2021 at 7:10 AM Itamar Turner-Trauring < > >>>>>>>>> ita...@pythonspeed.com> wrote: > >>>>>>>>> > >>>>>>>>>> Hi, > >>>>>>>>>> > >>>>>>>>>> Since the PR for high-level C++ Parquet encryption API appears > >>>>>> stalled > >>>>>>> ( > >>>>>>>>>> https://github.com/apache/arrow/pull/8023), I'm looking into > >>>>>> exposing > >>>>>>>>> the > >>>>>>>>>> low-level Parquet encryption API to Python. > >>>>>>>>>> > >>>>>>>>>> Arguments for doing this: the low-level API is all the users I'm > >>>>>>> talking > >>>>>>>>>> to need, at the moment, so it's plausible others would also find > >>>> some > >>>>>>>>>> benefit in having the Pyarrow API expose low-level Parquet > >>>>>> encryption. > >>>>>>>>> Then > >>>>>>>>>> again, it might only be this one company and no one else cares. > >>>>>>>>>> > >>>>>>>>>> The arguments against, per Gidon Gershinsky: > >>>>>>>>>> > >>>>>>>>>>> * security: low-level encryption API is easy to misuse (eg > >> giving > >>>>>> the > >>>>>>>>>> same keys for a number of different files; this'd break the AES > >> GCM > >>>>>>>>>> cipher). The high-level encryption layer handles that by > applying > >>>>>>>>> envelope > >>>>>>>>>> encryption and other best practices in data security. Also, this > >>>>>> layer > >>>>>>> is > >>>>>>>>>> maintained by the community, meaning that future improvements > and > >>>>>>>>> security > >>>>>>>>>> fixes can be upstreamed by anyone, and available to all. > >>>>>>>>>>> * compatibility: parquet-mr implements the high-level > encryption > >>>>>>>>> layer. > >>>>>>>>>> If we want the files produced by Spark/Presto/etc to be readable > >> by > >>>>>>>>>> pandas/PyArrow (and vice versa), we need to provide the Arrow > >> users > >>>>>>> with > >>>>>>>>>> the high-level API. > >>>>>>>>>>> ... > >>>>>>>>>>> > >>>>>>>>>>> The current situation is not ideal, it'd be good to merge the > >>>>>>>>> high-level > >>>>>>>>>> PR (and maybe hide the low level), but here we are; also, C++ > is a > >>>>>> kind > >>>>>>>>> of > >>>>>>>>>> a low-level language; Python would expose it to a less > experienced > >>>>>>>>> audience. > >>>>>>>>>> > >>>>>>>>>> (Source: https://issues.apache.org/jira/browse/ARROW-8040) > >>>>>>>>>> > >>>>>>>>>> I find the compatibility argument less compelling, that's > readily > >>>>>>>>>> addressed by documentation. I am not a crypto expert so I can't > >>>>>>> evaluate > >>>>>>>>>> how risky exposing the low-level encryption APIs would be, but I > >> can > >>>>>>> see > >>>>>>>>>> how that would be a significant concern. > >>>>>>>>>> > >>>>>>>>>> Some options are: > >>>>>>>>>> * Status quo, no Python API for low-level Parquet encryption. > >> This > >>>>>> has > >>>>>>>>>> two possible outcomes: > >>>>>>>>>> * Eventually high-level API gets merged, gets Python binding. > >>>>>>>>>> * High-level encryption API is never merged, Python users > never > >>>>>> get > >>>>>>>>>> access to encryption. > >>>>>>>>>> * Add low-level Parquet encryption API to Pyarrow, perhaps > using > >>>>>>>>> "hazmat" > >>>>>>>>>> idiom used by the Python cryptography package (API namespace > >>>>>> indicating > >>>>>>>>>> "use at your own risk, this is dangerous", basically, e.g. > >>>>>>>>>> > `cryptography.hazmat.primitives.ciphers.aead.``ChaCha20Poly1305`). > >>>>>>>>>> * Gidon Gershinsky did not find this suggestion compelling > >> enough > >>>>>> to > >>>>>>>>>> override his security concerns. > >>>>>>>>>> * Low-level encryption done as third party Python package, > either > >>>>>>>>> private > >>>>>>>>>> or open source. This is annoying technically, plausibly would > >>>> require > >>>>>>>>>> maintaining a fork. > >>>>>>>>>> Any other ideas? Thoughts on these options? > >>>>>>>>>> > >>>>>>>>>> —Itamar > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > > >
