ARROW-2760: [Python] Remove legacy property definition syntax from parquet module and test them by kszucs · Pull Request #2187 · apache/arrow

kszucs · 2018-06-28T15:06:56Z

No description provided.

kszucs · 2018-06-28T15:09:35Z

python/pyarrow/tests/test_parquet.py

The current implementation returns with zero distinct_count and INT64 for all of the integer types.
I'm not sure about the correctness of these values, can someone confirm (my doubt)?

This surprises me a bit. I would have expected that the small int types would have INT32 as their physical type.

@xhochy Which means INT64 is the expected? :)
How about distinct_count?

No, INT64 is not the expected. distinct_count was not computed, so I would expect np.nan or None as the result.

I guess I need to dig deeper than to find why the physical_types are incorrect.

kszucs · 2018-06-28T15:10:13Z

python/pyarrow/_parquet.pyx

Shouldn't this be called pshysical_type like in RowGroupStatistics?

Yes that would be much better

kszucs · 2018-07-04T10:44:03Z

@xhochy How do I trigger distinct_count computation?

Actually I can't find any parquet-cpp tests mentioning distinct_count.

AFAICS distinct_count is set along with num_values and null_count.

xhochy · 2018-07-04T10:46:44Z

We don't have that implemented yet AFAIK so the field should be set (i.e. we should return None)

kszucs · 2018-07-04T10:49:03Z

@xhochy wouldn't it be better to raise a NotImplementedError for stat.distinct_count instead?

xhochy · 2018-07-04T10:51:08Z

No, we could have distinct_count set when we load Parquet files that were written by parquet-mr. Just the ones written by parquet-cpp / pyarrow don't have it.

kszucs · 2018-07-04T10:55:51Z

I guess here we should check for has_distinct_count then, how should I represent missing distinct_count on the cpp API? Expose HasDistinctCount()?

kszucs · 2018-07-04T11:03:09Z

According to the has flags I should expose both HasNullCount and HasDistinctCount to be able to return None for stat.null_count and stat.distinct_count.

codecov-io · 2018-07-04T11:09:47Z

Codecov Report

Merging #2187 into master will increase coverage by 0.01%.
The diff coverage is 72.26%.

@@            Coverage Diff             @@
##           master    #2187      +/-   ##
==========================================
+ Coverage   84.45%   84.47%   +0.01%     
==========================================
  Files         293      293              
  Lines       45064    45113      +49     
==========================================
+ Hits        38061    38111      +50     
+ Misses       6972     6971       -1     
  Partials       31       31

Impacted Files	Coverage Δ
python/pyarrow/parquet.py	`91.28% <100%> (ø)`	⬆️
python/pyarrow/tests/test_parquet.py	`97.4% <100%> (+0.1%)`	⬆️
python/pyarrow/_parquet.pyx	`68.95% <64.48%> (-0.03%)`	⬇️
python/pyarrow/formatting.py	`68.75% <83.33%> (+25%)`	⬆️
cpp/src/arrow/util/thread-pool-test.cc	`98.91% <0%> (-0.55%)`	⬇️

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 8e6af29...d6a7f77. Read the comment docs.

xhochy · 2018-07-08T18:08:06Z

According to the has flags I should expose both HasNullCount and HasDistinctCount to be able to return None for stat.null_count and stat.distinct_count.

👍

pitrou · 2018-07-12T11:56:11Z

python/pyarrow/_parquet.pyx

This will return an iterator on Python 3, right? How about returning an actual container (dict or list)?

pitrou · 2018-07-12T11:57:54Z

python/pyarrow/_parquet.pyx

How about __ne__? Is it generated automatically by Cython?

Python's default implementation is for __ne__ is not __eq__

That seems true on Python 3, but not on Python 2:

Python 2.7.12 (default, Dec 4 2017, 14:50:18) [GCC 5.4.0 20160609] on linux2 Type "help", "copyright", "credits" or "license" for more information. >>> class C(object): ... def __eq__(self, other): return True ... >>> a = C() >>> b = C() >>> a == b True >>> a != b True >>>

Python 3.6.5 |Anaconda, Inc.| (default, Mar 29 2018, 13:14:23) Type 'copyright', 'credits' or 'license' for more information IPython 6.4.0 -- An enhanced Interactive Python. Type '?' for help. In [1]: class E: ...: def __eq__(self, other): ...: return True ...: In [5]: a = E() In [6]: b = E() In [7]: a == b Out[7]: True In [8]: a != b Out[8]: False

I guess this is a python 2/3 incompatibility. Cython does handle this one, right?

I don't know, best would be to add a test to exercise that :-)

Here is a test for ColumnSchema

pitrou · 2018-07-12T11:59:47Z

python/pyarrow/tests/test_parquet.py

I'm curious, why is distinct_count always None in the examples below?

Uwe said:

distinct_count was not computed, so I would expect np.nan or None as the result.

I see, can you add a small comment?

Will do, just doing some packaging stuff.

wesm · 2018-07-24T20:53:53Z

Can you rebase this?

…column stat properties

…ectly __cinit__ constructors instead of custom init methods

kszucs · 2018-07-25T09:13:34Z

python/pyarrow/_parquet.pyx

-            return self.metadata.index_page_offset()
+    @property
+    def dictionary_page_offset(self):
+        return self.metadata.dictionary_page_offset()


@xhochy Should it return None if has_dictionary_page is False?

kszucs · 2018-07-25T09:14:44Z

python/pyarrow/_parquet.pyx

-            return self.metadata.total_compressed_size()
+    @property
+    def index_page_offset(self):
+        return self.metadata.index_page_offset()


Similarly index_page_offset is 0, is it supposed to be zero, or index_page is optional too? (I'm not that familiar with parquet specs yet)

index_page is optional. This should return None.

How do I know that index_page hasn't been set? (index_page_offset == 0) => return None?

@xhochy ping

No, this is buggy. We should export it at the moment. Please raise NotImplementedError for now. I'll fix the parquet-cpp part over at https://issues.apache.org/jira/browse/PARQUET-1358

Do You mean:

@property def has_index_page(self): raise NotImplementedError @property def index_page_offset(self): # leave it as is return self.metadata.index_page_offset()

or

@property def index_page_offset(self): raise NotImplementedError

?

Files not written with parquet-cpp will lead to garbage on index_page_offset currently as it is mostly not set.

@property def has_index_page(self): raise NotImplementedError("not supported in parquet-cpp") @property def index_page_offset(self): raise NotImplementedError("parquet-cpp doesn't return valid values")

Ok, Thanks!

kszucs · 2018-07-25T09:24:32Z

python/pyarrow/tests/test_parquet.py


 def test_parquet_metadata_api():
+    import pyarrow.parquet as pq
+    import pyarrow._parquet as _pq


Should I expose the missing classes from pyarrow._parquet to pyarrow.parquet?

Yes, they seem to be helpful, so a user could also use them.

kszucs · 2018-07-25T10:03:18Z

According to the has flags I should expose both HasNullCount and HasDistinctCount to be able to return None for stat.null_count and stat.distinct_count.

@xhochy Do We need to release a new parquet-cpp version too with arrow=0.10.0? If not, then I wouldn't change parquet's API before the release.

xhochy · 2018-07-25T12:54:28Z

We will release a new parquet-cpp version once Arrow 0.10.0 is released. We should not change/break anything in parquet-cpp before the Arrow 0.10.0 release.

parquet-cpp releases are much simpler, so I can do more. The releases are mainly limited by PMCs that vote on the release.

kszucs · 2018-07-25T12:58:19Z

So until We have HasNullCount and HasDistinctCount I'll assert stat.distinct_count == 0 instead of None with a note.

wesm · 2018-07-27T00:42:25Z

This seems ready to go, I moved this into the 0.10.0 milestone so it gets merged

xhochy

+1, LGTM

Leaving this open for @pitrou for a final review

kszucs commented Jun 28, 2018

View reviewed changes

kszucs force-pushed the cython_properties branch from 669bf90 to f402963 Compare July 4, 2018 10:29

pitrou requested changes Jul 12, 2018

View reviewed changes

kszucs added 9 commits July 25, 2018 10:15

replace old property syntax; move indent to formatting; test missing …

c0e2a08

…column stat properties

remove enum cpdef

06e4f8e

due to receiving extension class as argument it's possuble to use dir…

c58441a

…ectly __cinit__ constructors instead of custom init methods

implement equality operator on ParquetSchema and ColumnSchema

266a022

warn instead of print

a43153e

rename type property to physical_type

dd36229

fix types

d55b96a

expected distinct_count value is None

ffa1041

test and fix column chunk metadata properties

1e1c7cd

kszucs force-pushed the cython_properties branch from 14c37e8 to 1e1c7cd Compare July 25, 2018 09:12

kszucs commented Jul 25, 2018

View reviewed changes

comments in compare schemas

6942eba

kszucs commented Jul 25, 2018

View reviewed changes

kszucs added 2 commits July 26, 2018 12:52

expose missing parquet classes to pq namespace

40ba965

missing distinct_count equals to zero

74d53bb

kszucs force-pushed the cython_properties branch from f037ead to 74d53bb Compare July 26, 2018 12:37

kszucs added 4 commits July 26, 2018 14:47

flake8

71f5ede

raise NotImplementedError for index_page_offset

b1e7bed

return NotImplemented from equality check

77c59d6

remove accidentally committed test helper script

d6a7f77

xhochy approved these changes Jul 27, 2018

View reviewed changes

pitrou approved these changes Jul 27, 2018

View reviewed changes

wesm closed this in 49ccf6a Jul 27, 2018

Conversation

kszucs commented Jun 28, 2018

Uh oh!

kszucs Jun 28, 2018 • 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!

kszucs commented Jul 4, 2018

Uh oh!

xhochy commented Jul 4, 2018

Uh oh!

kszucs commented Jul 4, 2018

Uh oh!

xhochy commented Jul 4, 2018

Uh oh!

kszucs commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kszucs commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xhochy commented Jul 8, 2018

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!

wesm commented Jul 24, 2018

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!

kszucs Jul 25, 2018 • 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

kszucs Jun 28, 2018 •

edited

Loading

kszucs commented Jul 4, 2018 •

edited

Loading

kszucs commented Jul 4, 2018 •

edited

Loading

codecov-io commented Jul 4, 2018 •

edited

Loading

kszucs Jul 25, 2018 •

edited

Loading

kszucs commented Jul 25, 2018 •

edited

Loading