Repository navigation
Conversation
raulcd
left a comment
There was a problem hiding this comment.
Thanks for the PR. A couple of things, could you use our PR template instead of removing the content? Could you also create an issue for this? Unfortunately this does not fit our MINOR definition as it changes a code file, not only docs and we require an issue.
Thanks.
As for the fix, I am wondering whether we could make this list being retrieved from the specific supported codecs programmatically instead of having a hardcoded list to avoid it from drifting in the future.
show_info() prints its Compression Codecs section from a hardcoded list that has drifted from the names _ensure_compression() actually accepts. "lz4" and "lz4_frame" both map to CCompressionType_LZ4_FRAME, so the same codec was reported twice under two names, while "lz4_raw", which maps to CCompressionType_LZ4 and is a separate codec, was not reported at all. Seven lines were printed for six codecs, and the seventh codec pyarrow supports was missing. List "lz4" once and add "lz4_raw".
e22f43e to
95365e4
Compare
|
Both of those are done — filed #52024 for the bug and retitled this accordingly, and the description now follows the template. The remaining question is yours: deriving the list programmatically rather than hardcoding it. I agree that is the better fix, and the obstacle is that the canonical mapping lives in |
|
|
Rationale for this change
pa.show_info()prints its Compression Codecs section from a hardcoded list:That list has drifted from
_ensure_compression()inio.pxi, which is whatactually decides which codec names work. Two problems follow:
lz4andlz4_frameboth map toCCompressionType_LZ4_FRAME, so the samecodec is reported twice under two names.
lz4_rawmaps toCCompressionType_LZ4and is a separate codec, but it isnot listed at all, even though
pa.Codec.is_available('lz4_raw')is True.So seven lines are printed for six codecs, and the codec that is missing is a
real one.
Before:
After:
What changes are included in this PR?
One list in
python/pyarrow/__init__.py: drop the duplicatelz4_frameand addthe missing
lz4_raw.lz4is kept as the name for the frame codec since thatis the one the docstrings lead with (
'lz4' (or 'lz4_frame')).On deriving this programmatically rather than hardcoding it — agreed, that is
the better fix. The canonical mapping lives in
_ensure_compression()inio.pxi, and neitherCodecnor the C++compression.hcurrently exposes theset of supported codecs, so it needs a new accessor on the Cython side for
show_info()to read. I am happy to do that here, with the caveat that I cannotcompile the Cython extension in my environment and would be relying on CI to
verify it. If you would prefer, I can keep this change minimal and open a
follow-up issue for the programmatic version. Let me know which you would rather
have.
Are these changes tested?
Not by a new test.
show_info()has no existing test coverage and its output isa diagnostic print, so I did not want to pin the exact text without knowing
whether you would want that — happy to add one if you do.
Verified by applying the same change to an installed pyarrow and reading the
output back, and by walking every name
_ensure_compression()accepts toconfirm which of them map onto the same C++ enum value:
Are there any user-facing changes?
Only the output of
pa.show_info(), which now lists each supported codec onceand includes
lz4_raw. No API change.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: