Add Parquet serialization of Tables with pyarrow by erykoff · Pull Request #12215 · astropy/astropy

erykoff · 2021-09-29T18:55:45Z

Description

This PR adds direct Parquet serialization of astropy Tables using pyarrow. Unlike serialization of Tables to Parquet via pandas, this functionality maintains all metadata, descriptions, and units; supports mixins (including Time and SkyCoord columns); and supports masked columns. An advantage of the Parquet format is the columnar access, and this PR allows efficient reading of subsets of columns from the serialized Table, using the proper Table column names (so the user does not have to worry about the deconstruction into components). The parquet reader also offers the option to read just a "schema" header, to obtain all the column, unit, and description information from the Table prior to performing i/o on a specific subset of columns. Finally, pyarrow filters are supported for reading Tables.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

github-actions

Welcome to Astropy 👋 and congratulations on your first pull request! 🎉

A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.

If you feel that this pull request has not been responded to in a timely manner, please leave a comment mentioning our software support engineer @embray, or send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail [email protected].

pllim · 2021-09-29T19:02:02Z

Also needs a change log, please see https://github.com/astropy/astropy/blob/main/docs/changes/README.rst

mhvk · 2021-09-29T20:33:51Z

This looks very nice! Though as I'm less familiar with the I/O interfaces, I'll leave review to @taldcroft (who is on holiday this week, so it may need a bit of patience). In the meantime, the docs failure seems real:

<rst_epilog>:126: WARNING: Duplicate explicit target name: "pyarrow".

Might as well fix that before running all the tests!

erykoff · 2021-10-02T21:35:59Z

I can't figure out what the build failure is for readthedocs https://readthedocs.org/projects/astropy/builds/14857984/

nstarman · 2021-10-02T23:57:36Z

@erykoff I believe the line is

/home/docs/checkouts/readthedocs.org/user_builds/astropy/checkouts/12215/docs/install.rst:201: WARNING: Undefined substitution referenced: "minimum_pyarrow_version".

We have RTD set to error on warnings, except for known issues like the Matplotlib ones.
In this case the substitution minimum_pyarrow_version is not defined.

erykoff · 2021-10-03T18:44:38Z

There were some CI errors on example code formatting that were unrelated to this PR. So I rebased to main, hopefully those were known problems that have been fixed since I had originally forked my branch.

erykoff · 2021-10-03T20:11:14Z

Errors are still there. Don’t know what to do about this:

027     >>> from astropy.io import fits
1182
028     >>> filename = fits.util.get_testdata_filepath('ascii.fits')
1183
029     >>> hdul = fits.open(filename)
1184
030     >>> hdul[1].data[:1]  # doctest: +FLOAT_CMP
1185
Expected:
1186
    FITS_rec([(10.123, 37)],
1187
             dtype=(numpy.record, {'names':['a','b'], 'formats':['S10','S5'], 'offsets':[0,11], 'itemsize':16}))
1188
Got:
1189
    FITS_rec([(10.123, 37)],
1190
             dtype=(numpy.record, {'names': ['a', 'b'], 'formats': ['S10', 'S5'], 'offsets': [0, 11], 'itemsize': 16}))
1191

nstarman · 2021-10-04T00:19:05Z

That is not your PR. I’m seeing the same error in one of mine. A recent PR or dependency update must have introduced this issue. Please check back in a day or two and we should have this sorted.

taldcroft · 2021-10-04T10:41:08Z

@erykoff - thanks, this is exciting! I'm just back from almost 2 weeks off and have a backlog of stuff to catch up so it might take a little bit to find time for a detailed review.

In the meantime, I see there are a lot of code coverage issues noted in the diffs, so you can take the opportunity to clean those up with additional testing.

erykoff · 2021-10-04T12:15:27Z

Ah, thanks for pointing out the coverage. What seems to have happened is that all my tests were skipped. I didn’t realize this because I was focused on the unrelated failing tests.

Looking further I see it never got to the “all optional dependencies” tests because of the docstring failure. So once that clears up I believe my coverage will increase considerably.

nstarman · 2021-10-04T14:13:36Z

@erykoff, I think if you rebase now the issue is resolved.

erykoff · 2021-10-04T15:25:50Z

Thank you so much! And the coverage seems much improved now that the tests are being run. 😄

nstarman

Thanks @erykoff for the PR! I'm not familiar with parquet serialization nor pyarrow, so @taldcroft's review will be much more useful :). Here I'm just making some style suggestions.

taldcroft · 2021-10-05T15:36:04Z

I'm not familiar with parquet serialization nor pyarrow

I'm afraid that applies to me as well but hopefully that won't stop us from getting this PR to a good place. @nstarman you are now the expert on the unified I/O layer, which is a big part of this.

Co-authored-by: Nathaniel Starkman <[email protected]>

nstarman · 2021-10-18T21:38:21Z

+
+One big advantage of the Parquet files is that each column is stored independently,
+and thus reading a subset of columns is fast and efficient.  To find out which
+columns are stored in a table without reading the data, use the ``schema_only=True``::


@taldcroft, I think schema_only might be a useful addition to bring to all table readers, like overwrite is available for all writers. Perhaps a goal for v5.1/2?

@nstarman - sure, that could be a goal. Another idea on that line would be uniform support for include/exclude names to avoid reading columns that are not needed. The benefit of that will depend a fair bit on the specifics of the reader. E.g. for HDF5 it might help a lot, while for ASCII it is basically not possible in the current framework to not read the other columns before dropping them.

Even with fits, a row-based format, columnar access is inefficient. But it sometimes is convenient to end up with a table that has only a subset of columns even if the read time and peak memory usage isn't significantly reduced. But with all of these formats, being able to just inspect the schema without reading the table would be useful!

Co-authored-by: Tom Aldcroft <[email protected]>

taldcroft

Thanks @erykoff, nice addition! You will have one final follow-up action as this definitely counts as a feature to highlight in the What's New in 5.0 release notes.

github-actions Bot added installation io.misc table testing time utils utils.masked labels Sep 29, 2021

pllim added this to the v5.0 milestone Sep 29, 2021

github-actions Bot reviewed Sep 29, 2021

View reviewed changes

pllim reviewed Sep 29, 2021

View reviewed changes

Comment thread setup.cfg

pllim added the whatsnew-needed label Sep 29, 2021

pllim requested review from mhvk and taldcroft September 29, 2021 19:50

mhvk removed their request for review September 29, 2021 20:34

erykoff force-pushed the u/erykoff/parquet branch from 6d8988b to 463273d Compare October 3, 2021 16:30

nstarman mentioned this pull request Oct 4, 2021

temporary patch for #12230 #12231

Merged

9 tasks done

erykoff force-pushed the u/erykoff/parquet branch from 463273d to 6bb8bec Compare October 4, 2021 14:15

nstarman reviewed Oct 4, 2021

View reviewed changes

erykoff and others added 17 commits October 18, 2021 08:54

Add parquet serialization code.

8faf846

Add parquet serialization tests.

308eb67

Add pyarrow optional dependency to documentation.

c5141d2

Add changelog snippet.

2be09a7

Apply suggestions from code review

fa9848a

Co-authored-by: Nathaniel Starkman <[email protected]>

Add review updates.

11e6d24

Respond to comments and improve test coverage.

7aa8934

Apply suggestions from code review

e238807

Co-authored-by: Nathaniel Starkman <[email protected]>

Add additional tests and improve code and test comments.

621ad5f

Apply suggestions from code review

cb8b125

Co-authored-by: Nathaniel Starkman <[email protected]>

Respond to more review comments.

a85cf64

Add include_names and exclude_names read options.

2087bd1

Add explicit tests for parquet_identify.

26f50dd

Fix flake8 errors.

3d29594

Apply suggestions from code review

2601ec9

Co-authored-by: Nathaniel Starkman <[email protected]>

Add support for writing to path-like outputs.

5390e5c

Apply suggestions from code review

04990de

Co-authored-by: Nathaniel Starkman <[email protected]>

erykoff force-pushed the u/erykoff/parquet branch from 4cdfbd2 to 04990de Compare October 18, 2021 15:54

erykoff added 2 commits October 18, 2021 09:21

Update imports and overwrite message

d08a505

Add parquet to documentation.

52a7932

nstarman reviewed Oct 18, 2021

View reviewed changes

taldcroft reviewed Oct 19, 2021

View reviewed changes

Comment thread docs/io/unified.rst Outdated

Comment thread docs/io/unified.rst Outdated

Comment thread docs/io/unified.rst Outdated

Comment thread docs/io/unified.rst Outdated

Comment thread docs/io/unified.rst Outdated

Apply suggestions from code review

cc34a66

Co-authored-by: Tom Aldcroft <[email protected]>

nstarman requested a review from taldcroft October 21, 2021 03:55

taldcroft approved these changes Oct 21, 2021

View reviewed changes

taldcroft merged commit 77a7577 into astropy:main Oct 21, 2021

erykoff mentioned this pull request Oct 25, 2021

Add Parquet support to 5.0 what's new. #12291

Merged

8 of 9 tasks

pllim mentioned this pull request Oct 26, 2021

Parquet v2.0 deprecated #12298

Closed

pllim mentioned this pull request Nov 11, 2022

Parquet connector does not support writing variable length arrays #14000

Closed

pllim mentioned this pull request May 4, 2026

BUG: a new incompatibility showing up directly on stable CI #19651

Closed

Conversation

erykoff commented Sep 29, 2021 • edited by taldcroft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist for package maintainer(s)

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pllim commented Sep 29, 2021

Uh oh!

mhvk commented Sep 29, 2021

Uh oh!

erykoff commented Oct 2, 2021

Uh oh!

nstarman commented Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erykoff commented Oct 3, 2021

Uh oh!

erykoff commented Oct 3, 2021

Uh oh!

nstarman commented Oct 4, 2021

Uh oh!

taldcroft commented Oct 4, 2021

Uh oh!

erykoff commented Oct 4, 2021

Uh oh!

nstarman commented Oct 4, 2021

Uh oh!

erykoff commented Oct 4, 2021

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft commented Oct 5, 2021

Uh oh!

nstarman Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

taldcroft Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

erykoff Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

erykoff commented Sep 29, 2021 •

edited by taldcroft

Loading

nstarman commented Oct 2, 2021 •

edited

Loading