ARROW-6667: [Python] remove cyclical object references in pyarrow.parquet

[AaronOpfer]

_build_nested_path has a reference cycle because the closured function refers to the parent cell which also refers to the closured function again. Address this by clearing the reference to the function from the parent cell before returning.

open_dataset_file is partialed with self inside the ParquetFile class. Prevent this by using a weakref instead.

[pitrou]

Why do you care about _build_nested_path exactly? It's just a small function, it's not a problem if there's a reference cycle.

_open_dataset looks more serious, since it may keep alive an entire dataset.

[pitrou]

As the CI shows, this PR breaks the unit tests. I recommend you run the unit tests using pytest before pushing changes, since it will allow you to iterate more quickly.

In this case, the weakref breaks pickling. I would suggest trying to untangle the mess that is the _open_dataset_file indirection, perhaps by moving some functionality in a dedicated DatasetFileOpener (or DatasetParams?) class that would hold the necessary parameters.

[AaronOpfer]

Hi, sorry for not catching the test failures in the first push.

I would suggest trying to untangle the mess that is the _open_dataset_file indirection, perhaps by moving some functionality in a dedicated DatasetFileOpener (or DatasetParams?) class that would hold the necessary parameters.

The new patch creates a metadata storage object for the purpose of breaking the cycle in ParquetDataset. The implementation of which I'm not super pleased about, so I'm interested in what your feedback is there.

Why do you care about _build_nested_path exactly? It's just a small function, it's not a problem if there's a reference cycle.

I'm analyzing my codebase trying to identify why my garbage collector is running very frequently during calculations and my analysis had me find this reference cycle. I would be uncomfortable raising my garbage collection threshold or disabling it outright for performance gains unless my analysis actually shows that I never actually collect any garbage, and the _build_nested_path stackframes are swept on every collection. If I can read between the lines a bit, I agree with you that it is a minor problem when it comes to actual bytes of memory wasted.

[pitrou]

I'm analyzing my codebase trying to identify why my garbage collector is running very frequently during calculations and my analysis had me find this reference cycle.

Hmm, I'd suggest taking some steps back :-) The Python garbage collector is running frequently by design. However, there are different kinds of garbage collections: incremental and full garbage collections. Only the full garbage collections can be costly, and even then, in most applications they have a minimal cost. You may use gc.callbacks if you want to find out more information about this.

Generally, it's expected for object-oriented code (and/or code using closures, etc.) to have reference cycles. As long as those reference cycles don't keep a costly object alive (such as an open file or a large array), it shouldn't be much of a problem.

[AaronOpfer]

Looks like Python 2.7 can't pickle bound methods. ☹️ I'll go ahead and make the code uglier to fix it.

[AaronOpfer]

BTW @pitrou just realized you've had to come in after my garbage collection changes before...

tornadoweb/tornado#1782
tornadoweb/tornado#2230

Small world isn't it? 😃 This time you managed to stop me before I added weakrefs instead of after.

[wesm]

Looks like this is passing save for lint issues. I'm going to fix your lint issues in hopes that @pitrou can merge this in the morning before a 0.15.0 release candidate is cut

[wesm]

Please make pull requests using branches in the future, it's tedious and error prone for us to push commits to your master branch

[wesm]

I can't rebase/force-push otherwise GitHub will force the PR closed

[pitrou]

LGTM. I'll wait for Travis-CI to pass and then merge.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@c6faaed). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #5476   +/-   ##
=========================================
  Coverage          ?   66.35%           
=========================================
  Files             ?      508           
  Lines             ?    70129           
  Branches          ?        0           
=========================================
  Hits              ?    46533           
  Misses            ?    23596           
  Partials          ?        0

Impacted Files	Coverage Δ
python/pyarrow/parquet.py	92.5% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6faaed...f4909e0. Read the comment docs.