You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "milesgranger (via GitHub)" <gi...@apache.org> on 2024/01/24 13:29:53 UTC
[PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
milesgranger opened a new pull request, #39781:
URL: https://github.com/apache/arrow/pull/39781
I think the hash, especially for `FileMetaData` could be better, maybe just use return of `__repr__`, even though that won't include row group info?
### Rationale for this change
Helpful for dependent projects.
### What changes are included in this PR?
Impl `__hash__` for `ParquetSchema` and `FileMetaData`
### Are these changes tested?
Yes
### Are there any user-facing changes?
Supports hashing metadata:
```python
In [1]: import pyarrow.parquet as pq
In [2]: f = pq.ParquetFile('test.parquet')
In [3]: hash(f.metadata)
Out[3]: 4816453453708427907
In [4]: hash(f.metadata.schema)
Out[4]: 2300988959078172540
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1486077014
##########
cpp/submodules/parquet-testing:
##########
Review Comment:
Also, can you rebase on latest git main?
##########
cpp/submodules/parquet-testing:
##########
Review Comment:
It seems this is a spurious change, can you update the submodules on your branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480181089
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
I think it would be simpler, and probably faster, to serialize the metadata using `self._metadata.WriteTo` and then hash the resulting buffer contents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #39781:
URL: https://github.com/apache/arrow/pull/39781#issuecomment-1910365749
> I think the hash, especially for FileMetaData could be better, maybe just use return of __repr__, even though that won't include row group info?
I am not sure if row group info is necessary - do not have much experience with `hash()`. But naively I would think using `__repr__` would be enough. @raulcd what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39781:
URL: https://github.com/apache/arrow/pull/39781#issuecomment-1938995318
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 56d1ec1142cd95f1c2d35805396262713fa5b29f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/21484296202) has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480266828
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
As a general basis, a `__hash__` does not need to take all contents exhaustively into account if doing so is too expensive. It's ok to limit ourselves to a smaller, faster subset. For example only the schema:
```python
>>> %timeit hash(repr(md.schema))
6.59 µs ± 27.3 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
```
You can also convert to a Arrow schema:
```python
>>> %timeit md.schema.to_arrow_schema()
4.94 µs ± 8.25 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
````
Unfortunately, `pa.Schema.__hash__` is too slow currently:
```python
>>> s = md.schema.to_arrow_schema()
>>> %timeit hash(s)
12.7 µs ± 25.9 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
```
This is because `pa.Schema.__hash__` takes the [slow Python way](https://github.com/apache/arrow/blob/a6e577d031d20a1a7d3dd01536b9a77db5d1bff8/python/pyarrow/types.pxi#L2524-L2525), while the C++ Schema has a useful and fast `fingerprint` method. (same comment for `pa.Field` and `pa.DataType)
Of course you don't have to do all this here, but a `__hash__` function that takes milliseconds to execute is definitely not a good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #39781:
URL: https://github.com/apache/arrow/pull/39781#issuecomment-1910104530
Hi Miles! 👋
The CI failures have just been fixed so would go away if you rebase.
Will look at the PR shortly 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "milesgranger (via GitHub)" <gi...@apache.org>.
milesgranger commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1485993573
##########
python/pyarrow/_parquet.pyx:
##########
@@ -1071,6 +1083,11 @@ cdef class ParquetSchema(_Weakrefable):
def __getitem__(self, i):
return self.column(i)
+ def __hash__(self):
+ return hash(
+ (object.__hash__(self), frombytes(self.schema.ToString(), safe=True))
Review Comment:
Addressed in https://github.com/apache/arrow/pull/39781/commits/45059380531161759fe093281ff97988c9207bca
##########
python/pyarrow/tests/parquet/test_metadata.py:
##########
@@ -499,6 +499,24 @@ def test_multi_dataset_metadata(tempdir):
assert md['serialized_size'] > 0
+def test_metadata_hashing(tempdir):
+ path1 = str(tempdir / "metadata1")
+ schema1 = pa.schema([("a", "int64"), ("b", "float64")])
+ pq.write_metadata(schema1, path1)
+ parquet_meta1 = pq.read_metadata(path1)
+
+ path2 = str(tempdir / "metadata2")
+ schema2 = pa.schema([("a", "int64"), ("b", "float32")])
+ pq.write_metadata(schema2, path2)
+ parquet_meta2 = pq.read_metadata(path2)
+
+ # Deterministic
+ assert hash(parquet_meta1) == hash(parquet_meta1)
Review Comment:
Done in https://github.com/apache/arrow/pull/39781/commits/45059380531161759fe093281ff97988c9207bca, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #39781:
URL: https://github.com/apache/arrow/pull/39781#issuecomment-1908127062
:warning: GitHub issue #39780 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #39781:
URL: https://github.com/apache/arrow/pull/39781#issuecomment-1938683712
Thank you @milesgranger !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "milesgranger (via GitHub)" <gi...@apache.org>.
milesgranger commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1485993234
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
Thanks for confirming my suspicion the current approach was abysmal. Just wasn't sure _what_ belongs in there. At least schema but seemed like filemetadata was a superset in some way of schema, so ought to include a few other things, thus added a few more attributes in the followup; please feel free to suggest any changes there. :)
Timings I have for https://github.com/apache/arrow/pull/39781/commits/45059380531161759fe093281ff97988c9207bca are:
```python
In [1]: import pyarrow.parquet as pq
In [2]: meta = pq.read_metadata("../../lineitem-stats-no-index.snappy.parquet")
In [3]: schema = meta.schema.to_arrow_schema()
In [4]: %timeit hash(meta)
4.34 µs ± 12.2 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
In [5]: %timeit hash(schema)
8.34 µs ± 31.2 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "milesgranger (via GitHub)" <gi...@apache.org>.
milesgranger commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1486173478
##########
cpp/submodules/parquet-testing:
##########
Review Comment:
Done and done. :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480273615
##########
python/pyarrow/_parquet.pyx:
##########
@@ -1071,6 +1083,11 @@ cdef class ParquetSchema(_Weakrefable):
def __getitem__(self, i):
return self.column(i)
+ def __hash__(self):
+ return hash(
+ (object.__hash__(self), frombytes(self.schema.ToString(), safe=True))
Review Comment:
Two things:
1. `object.__hash__` hashes by identity, which is precisely what you want to avoid here:
```python
>>> s = md.schema.to_arrow_schema()
>>> s2 = md.schema.to_arrow_schema()
>>> object.__hash__(s)
8763812462659
>>> object.__hash__(s2)
8763812583391
```
2. `frombytes` is not necessary, you can just hash a bytes object directly, which should be slightly faster
```python
>>> s.to_string()
'l_orderkey: int32\nl_partkey: int32\nl_suppkey: int32\nl_linenumber: int32\nl_quantity: double\nl_extendedprice: double\nl_discount: double\nl_tax: double\nl_returnflag: string\nl_linestatus: string\nl_shipdate: timestamp[us]\nl_commitdate: timestamp[us]\nl_receiptdate: timestamp[us]\nl_shipinstruct: string\nl_shipmode: string\nl_comment: string'
>>> b = s.to_string().encode()
>>> %timeit hash(b)
56.4 ns ± 3.83 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)
>>> %timeit hash(b.decode())
242 ns ± 2.15 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480240232
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
Note that it can be as simple as taking the hash of a `bytes` copy:
```python
>>> hash(stream.getvalue().to_pybytes())
5574314894425565867
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480191173
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
But then you probably want to make `pa.Buffer` hashable as well:
```python
>>> stream = pa.BufferOutputStream()
>>> stream.write(b"xxx")
3
>>> stream.getvalue()
<pyarrow.Buffer address=0x7f84af208000 size=3 is_cpu=True is_mutable=True>
>>> stream.getvalue() == b"xxx"
True
>>> hash(stream.getvalue())
Traceback (most recent call last):
Cell In[9], line 1
hash(stream.getvalue())
TypeError: unhashable type: 'pyarrow.lib.Buffer'
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480255098
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
But, even then, this approach will not be very performant while a hash is supposed to compute quickly:
```python
>>> md = pq.read_metadata("/home/antoine/arrow/dev/python/lineitem-snappy.pq")
>>> md
<pyarrow._parquet.FileMetaData object at 0x7f87befc19e0>
created_by: parquet-cpp-arrow version 15.0.0-SNAPSHOT
num_columns: 16
num_rows: 2568534
num_row_groups: 21
format_version: 2.6
serialized_size: 32440
>>> md.__reduce__()
(<cyfunction _reconstruct_filemetadata at 0x7f87c02fea80>,
(<pyarrow.Buffer address=0x7f88082086c0 size=32440 is_cpu=True is_mutable=True>,))
>>> hash(md.__reduce__()[1][0].to_pybytes())
8779236492631648618
>>> %timeit hash(md.__reduce__()[1][0].to_pybytes())
93.6 µs ± 144 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)
```
It will be far worse with this PR's approach, since generating the dict is extremely costly:
```python
>>> %timeit md.to_dict()
2.95 ms ± 29.2 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
```
(note: milliseconds, as opposed to microseconds above)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1480274554
##########
python/pyarrow/tests/parquet/test_metadata.py:
##########
@@ -499,6 +499,24 @@ def test_multi_dataset_metadata(tempdir):
assert md['serialized_size'] > 0
+def test_metadata_hashing(tempdir):
+ path1 = str(tempdir / "metadata1")
+ schema1 = pa.schema([("a", "int64"), ("b", "float64")])
+ pq.write_metadata(schema1, path1)
+ parquet_meta1 = pq.read_metadata(path1)
+
+ path2 = str(tempdir / "metadata2")
+ schema2 = pa.schema([("a", "int64"), ("b", "float32")])
+ pq.write_metadata(schema2, path2)
+ parquet_meta2 = pq.read_metadata(path2)
+
+ # Deterministic
+ assert hash(parquet_meta1) == hash(parquet_meta1)
Review Comment:
Ok, but should also check that two equal but distinct instances also hash equally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "milesgranger (via GitHub)" <gi...@apache.org>.
milesgranger commented on code in PR #39781:
URL: https://github.com/apache/arrow/pull/39781#discussion_r1485993234
##########
python/pyarrow/_parquet.pyx:
##########
@@ -849,6 +849,18 @@ cdef class FileMetaData(_Weakrefable):
cdef Buffer buffer = sink.getvalue()
return _reconstruct_filemetadata, (buffer,)
+ def __hash__(self):
+ def flatten(obj):
Review Comment:
Thanks for confirming my suspicion the current approach was abysmal. Just wasn't sure _what_ belongs in there. At least schema but seemed like filemetadata was a superset in some way of schema, so ought to include a few other things, thus added a few more attributes in the followup; please feel free to suggest any changes there. :)
Timings I have for https://github.com/apache/arrow/pull/39781/commits/45059380531161759fe093281ff97988c9207bca are:
```python
In [9]: %timeit hash(meta)
727 ns ± 2.69 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
In [10]: %timeit hash(schema)
1.2 µs ± 144 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39780: [Python][Parquet] Support hashing for FileMetaData and ParquetSchema [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #39781:
URL: https://github.com/apache/arrow/pull/39781
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org