ARROW-881: [Python] Reconstruct Pandas DataFrame indexes using metadata by cpcloud · Pull Request #612 · apache/arrow

cpcloud · 2017-04-29T00:21:09Z

cc @mrocklin

cpcloud · 2017-04-29T00:21:26Z

I'm going to revert these.

cpcloud · 2017-04-29T00:21:32Z

cpcloud · 2017-04-29T00:21:41Z

This is a rebase artifact.

cpcloud · 2017-04-29T00:22:18Z

This needs a test

is this cool now?

Let me add a small test, doing it now.

cpcloud · 2017-04-29T00:24:41Z

This limits us to MultiIndexes with <= 255 levels (because we're using string -> string for metadata). I think that's reasonable for now. We can always come with up a more complex encoding if we want to support more levels than that. I'd be surprised if this ever comes up in practice.

wesm · 2017-04-29T17:55:34Z

Maybe serialize_pandas and deserialize_pandas?

wesm · 2017-04-29T18:01:42Z

This function is getting "chubby" enough that we should probably move it to a pandas utility module in pure Python.

wesm · 2017-04-29T18:04:25Z

Same comment as above re: doing this in pure Python. It would also encourage adding appropriate public APIs to pyarrow.Table. We already have Table.remove_column, so it is probably better to use that if possible.

wesm · 2017-04-29T18:04:37Z

wesm · 2017-04-29T18:06:01Z

This DEFAULT_INDEX_FIELD is a slight nuisance. Perhaps add an argument to from_pandas whether to ingest the index (default could be True or False I guess)?

wesm · 2017-04-29T18:08:16Z

I think <= 255 levels is OK. I would actually rather see this metadata stored as a JSON blob under a single pandas key, otherwise we are possibly muddying the metadata namespace.

metadata = {b'pandas': json.dumps(pandas_meta).encode('utf8')}

wesm · 2017-04-29T18:08:56Z

PARQUET-595 is merged

cpcloud · 2017-05-14T02:35:35Z

@wesm This is ready for another round of review when you get a chance.

wesm · 2017-05-14T13:05:52Z

OK, taking a look now. Minor rebase conflict from #679

cpcloud · 2017-05-14T16:37:51Z

Fixed the conflict and addressed the {,RecordBatch}File{Reader,Writer} change.

wesm

Overall this looks fine, this will be very nice to have! I would say we should start factoring out code from pyarrow.lib that doesn't need to be cythonized, which will make iterative development a little easier in cases too

wesm · 2017-05-14T13:07:43Z

You should be able to call this and the one above in a const context. You'll have to mark name_to_index_ as mutable to make this work

wesm · 2017-05-14T19:18:11Z

is this cool now?

wesm · 2017-05-14T19:19:20Z

Maybe we should factor this out into a pandas_compat.py module, along with the rest of the stuff below

This is pretty awkward to factor out because of the TimeUnit_* enum values. We'd have to make pandas_compat.pxi if we wanted to keep those available to Cython but not Python (which would seem to defeat part of the purpose of factoring out) or expose the enum values to Python. This doesn't seem worth it for something that will never be seen by a user. Still, if you feel strongly about it I can spend some more time on it.

True true, no worries, this is fine as is.

wesm · 2017-05-14T19:21:48Z

I think we need an explicit read_pandas function in this class so that the user must express intent to use the additional pandas metadata

I think this is the last thing in this patch. I would like to have the option to ignore the metadata and read the file as-is as an Arrow table (without having the index columns tacked on against my will). So we can either add a read_pandas method to enables the metadata wrangling logic, or an option to read that does the same thing.

Yep, fully on board here. Just trying to iron out pandas_compat stuff, then moving on to this.

wesm · 2017-05-14T19:22:44Z

This is a regression, since pandas is not a hard dependency.

wesm · 2017-05-14T19:25:28Z

Move this some of this code to pyarrow.pandas_compat?

wesm · 2017-05-14T19:25:36Z

Use pyarrow_wrap_table here

wesm · 2017-05-14T19:28:18Z

Maybe make check_index default to false?

wesm · 2017-05-14T19:29:01Z

Test a MultiIndex here?

What is the behavior when the columns are not strings?

This now raises a TypeError alerting the user to the fact that column names cannot be anything other than strings.

Also added a multiindex test.

wesm · 2017-05-14T19:29:45Z

Document this extra parameter

wesm · 2017-05-15T14:20:36Z

I think this and #602 are the last things I'd like to get in before cutting 0.4.0 (outside some clean up patches).

cpcloud · 2017-05-15T18:09:31Z

Sounds good!

wesm · 2017-05-15T20:26:02Z

can you add nthreads=None here and pass through to to_pandas (single-threaded by default)

wesm · 2017-05-15T21:18:50Z

made a last comment #612 (comment) but outside of that i think this is about good to go

wesm · 2017-05-16T02:06:56Z

Need to call _get_column_indices on these?

Ah crap. Yep. Will also add a test since this wasn't failing for me locally.

…_metadata

wesm · 2017-05-16T17:13:42Z

Here's the appveyor build: https://ci.appveyor.com/project/cpcloud/arrow/build/1.0.158

+1, thanks for doing this!

@mrocklin

cc @mrocklin Author: Phillip Cloud <cpcloud@gmail.com> Closes apache#612 from cpcloud/ARROW-881 and squashes the following commits: 4fa679d [Phillip Cloud] Add metadata test 60f71aa [Phillip Cloud] More doc de616e8 [Phillip Cloud] Add doc a42a084 [Phillip Cloud] Decode metadata to utf8 because JSON 2198dc5 [Phillip Cloud] Call column_name_idx on index_columns 32c5e64 [Phillip Cloud] Add test for read_pandas subset 2fa1f16 [Phillip Cloud] Do not write index_column metadata if not requested 21a8829 [Phillip Cloud] Add docs to pq.read_pandas c35970c [Phillip Cloud] Add test for no index written and pq.read_pandas 59477b5 [Phillip Cloud] ARROW-881: [Python] Reconstruct Pandas DataFrame indexes using custom_metadata

cpcloud commented Apr 29, 2017

View reviewed changes

Comment thread cpp/src/arrow/ipc/metadata.cc Outdated

cpcloud Apr 29, 2017

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to revert these.

cpcloud commented Apr 29, 2017

View reviewed changes

Comment thread cpp/src/arrow/ipc/metadata.cc Outdated

cpcloud Apr 29, 2017

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too.

cpcloud commented Apr 29, 2017

View reviewed changes

Comment thread cpp/src/arrow/type.h Outdated

cpcloud Apr 29, 2017

Copy link
Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rebase artifact.

cpcloud commented Apr 29, 2017

View reviewed changes

wesm reviewed Apr 29, 2017

View reviewed changes

wesm reviewed May 14, 2017

View reviewed changes

wesm reviewed May 15, 2017

View reviewed changes

wesm reviewed May 16, 2017

View reviewed changes

cpcloud added 10 commits May 16, 2017 10:36

ARROW-881: [Python] Reconstruct Pandas DataFrame indexes using custom…

59477b5

…_metadata

Add test for no index written and pq.read_pandas

c35970c

Add docs to pq.read_pandas

21a8829

Do not write index_column metadata if not requested

2fa1f16

Add test for read_pandas subset

32c5e64

Call column_name_idx on index_columns

2198dc5

Decode metadata to utf8 because JSON

a42a084

Add doc

de616e8

More doc

60f71aa

Add metadata test

4fa679d

asfgit closed this in bed0197 May 16, 2017

cpcloud deleted the ARROW-881 branch May 16, 2017 18:54

Uh oh!

Conversation

cpcloud commented Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpcloud Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Apr 29, 2017

Uh oh!

cpcloud commented May 14, 2017

Uh oh!

wesm commented May 14, 2017

Uh oh!

cpcloud commented May 14, 2017

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpcloud commented Apr 29, 2017 •

edited

Loading

cpcloud Apr 29, 2017 •

edited

Loading

cpcloud May 15, 2017 •

edited

Loading