ARROW-1883: [Python] Fix handling of metadata in to_pandas when not all columns are present

[jorisvandenbossche]

This closes ARROW-1883.

So basically what I did in _add_any_metadata was replacing col = table[i] with:

idx = schema.get_field_index(raw_name)
if idx != -1:
     col = table[idx]

to check that the column is actually present in the schema. However, that involved some more code to get to raw_name (the name how the column is present in the schema), as this does not always match the name in pandas_metadata['column'][..]['name']. Not sure if there is a better way to get that name.
(or if it would be better to filter pandas_metadata earlier on, instead of checking when actually trying to process the metadata of that column)

[cpcloud]

The travis failure looks unrelated.

[cpcloud]

@jorisvandenbossche Thanks for reporting and fixing! Couple of minor things to address before merging.

[cpcloud]

can you pull pandas_metadata['index_columns'] out into a variable?

[cpcloud]

Can you add 2 tests: 1 against a datetime index and 1 against another index that isn't datetime? We need to make sure the i >= ncolumns condition is tested.

You'll need to pass preserve_index=True to pa.Table.from_pandas().

[jorisvandenbossche]

Yes, I was actually playing with such frames interactively testing it out, but forgot to add to the actual tests.

preserve_index=True is the default, but I can add it for explicitness if you want

[jorisvandenbossche]

Thanks for the review, added some more tests.

[wesm]

How does this patch align with #1397?

[cpcloud]

@wesm I believe @jorisvandenbossche said he was going to rebase on top of #1397 when that's merged

[jorisvandenbossche]

@wesm yes, and I have time to that today (or tomorrow) once the other is merged

[jorisvandenbossche]

I updated this now #1397 is merged. But, I could not really simplify the code to just use the new 'field_name' as I suppose we still want to be able to read in metadata written by older versions of arrow?

[jorisvandenbossche]

Added a test that a parquet file written by arrow 0.7.1 is correctly read.

@cpcloud @wesm @xhochy this is ready again for review

[jorisvandenbossche]

For reference, the test file is written as:

In [114]: pa.__version__
Out[114]: '0.7.1'

In [117]: df = pd.DataFrame({
     ...:             'a': [1, 2, 3],
     ...:             'b': [.1, .2, .3], 'c': pd.date_range("2017-01-01", periods=3, tz='Europe/Brussels')})

In [118]: df.index = pd.MultiIndex.from_arrays([['a', 'b', 'c'], pd.date_range("2017-01-01", periods=3, tz='Europe/Brussels')], names=['index', None])

In [119]: df
Out[119]: 
                                 a    b                         c
index                                                            
a     2017-01-01 00:00:00+01:00  1  0.1 2017-01-01 00:00:00+01:00
b     2017-01-02 00:00:00+01:00  2  0.2 2017-01-02 00:00:00+01:00
c     2017-01-03 00:00:00+01:00  3  0.3 2017-01-03 00:00:00+01:00

In [120]: import pyarrow.parquet as pq

In [123]: table = pa.Table.from_pandas(df)

In [124]: table
Out[124]: 
pyarrow.Table
a: int64
b: double
c: timestamp[ns, tz=Europe/Brussels]
index: string
__index_level_1__: timestamp[ns, tz=Europe/Brussels]
metadata
--------
{b'pandas': b'{"index_columns": ["index", "__index_level_1__"], "pandas_versio'
            b'n": "0.22.0.dev0+313.g7105339", "columns": [{"numpy_type": "int6'
            b'4", "metadata": null, "pandas_type": "int64", "name": "a"}, {"nu'
            b'mpy_type": "float64", "metadata": null, "pandas_type": "float64"'
            b', "name": "b"}, {"numpy_type": "datetime64[ns, Europe/Brussels]"'
            b', "metadata": {"timezone": "Europe/Brussels"}, "pandas_type": "d'
            b'atetimetz", "name": "c"}, {"numpy_type": "object", "metadata": n'
            b'ull, "pandas_type": "unicode", "name": "index"}, {"numpy_type": '
            b'"datetime64[ns, Europe/Brussels]", "metadata": {"timezone": "Eur'
            b'ope/Brussels"}, "pandas_type": "datetimetz", "name": "__index_le'
            b'vel_1__"}]}'}

In [125]: pq.write_table(table, 'v0.7.1.column-metadata-handling.parquet')

(similar code to construct the dataframe is in the test to create the expected result)

[wesm]

+1. We should probably remove the backwards compatibility code after some statute of limitations has passed (1 or 2 major releases) so we aren't maintaining it forever. Having this release available will give people the ability to "fix" their files if needed

[jorisvandenbossche]

We should probably remove the backwards compatibility code after some statute of limitations has passed

To make this cleaner, I could also move all back compat-handling code to a function that takes a metadata object and rewrites it in a new form, so the rest of the code doesn't need to care about it, and it is easier to remove later on. If you would be interested in that, I can do that here, or in a new PR (as that is not release critical)

[wesm]

No need to rock the boat here, I will merge this once the Appveyor build runs

[wesm]

thanks @jorisvandenbossche!!

[jorisvandenbossche]

You're welcome! And happy to have a first patch to arrow :)