Python bindings for Parquet select cols by field ID - #22956
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
…ect-cols-by-field-id
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Parquet Python bindings add reader option and builder support for selecting columns by field IDs, forward IDs as ChangesParquet field-ID projection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/pylibcudf/tests/io/test_parquet.py (1)
109-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding edge case tests for field-ID selection.
The test covers the happy path well, but per coding guidelines, test files should provide comprehensive edge case coverage. Consider adding cases for: empty field-ID list, single field-ID, non-existent field-ID (error handling), and duplicate field-IDs.
As per coding guidelines: "Ensure test files provide comprehensive edge case coverage (empty, all-null, single-element, mixed types) and do not depend on external datasets."
🧪 Suggested edge case tests
def test_read_parquet_column_field_ids(binary_source_or_sink): # ... existing test ... +def test_read_parquet_column_field_ids_single(binary_source_or_sink): + schema = pa.schema([ + pa.field("col_int64", pa.int64(), metadata={b"PARQUET:field_id": b"10"}), + pa.field("col_string", pa.string(), metadata={b"PARQUET:field_id": b"20"}), + ]) + pa_table = pa.Table.from_arrays( + [ + pa.array([1, 2, 3], type=pa.int64()), + pa.array(["a", "b", "c"], type=pa.string()), + ], + schema=schema, + ) + source = make_source( + binary_source_or_sink, pa_table, **_COMMON_PARQUET_SOURCE_KWARGS + ) + source_info = plc.io.SourceInfo([source]) + options = ( + plc.io.parquet.ParquetReaderOptions.builder(source_info) + .column_field_ids([20]) + .build() + ) + res = plc.io.parquet.read_parquet(options) + assert_table_and_meta_eq( + pa_table.select(["col_string"]), + res, + check_field_nullability=False, + ) + +def test_read_parquet_column_field_ids_empty(binary_source_or_sink): + schema = pa.schema([ + pa.field("col_int64", pa.int64(), metadata={b"PARQUET:field_id": b"10"}), + ]) + pa_table = pa.Table.from_arrays( + [pa.array([1, 2, 3], type=pa.int64())], + schema=schema, + ) + source = make_source( + binary_source_or_sink, pa_table, **_COMMON_PARQUET_SOURCE_KWARGS + ) + source_info = plc.io.SourceInfo([source]) + options = ( + plc.io.parquet.ParquetReaderOptions.builder(source_info) + .column_field_ids([]) + .build() + ) + res = plc.io.parquet.read_parquet(options) + # Assert behavior for empty field-ID list (all columns or no columns)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/pylibcudf/tests/io/test_parquet.py` around lines 109 - 155, Extend test_read_parquet_column_field_ids with edge-case coverage for an empty field-ID list, a single field ID, duplicate field IDs, and a non-existent field ID that verifies the expected error. Reuse the existing in-memory schema, source, and reader-options setup, and assert the resulting column selection or error behavior for each case without introducing external datasets.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@python/pylibcudf/tests/io/test_parquet.py`:
- Around line 109-155: Extend test_read_parquet_column_field_ids with edge-case
coverage for an empty field-ID list, a single field ID, duplicate field IDs, and
a non-existent field ID that verifies the expected error. Reuse the existing
in-memory schema, source, and reader-options setup, and assert the resulting
column selection or error behavior for each case without introducing external
datasets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee9f198a-fd6b-4f36-a64a-cf3f321ccedc
📒 Files selected for processing (5)
python/pylibcudf/pylibcudf/io/parquet.pxdpython/pylibcudf/pylibcudf/io/parquet.pyipython/pylibcudf/pylibcudf/io/parquet.pyxpython/pylibcudf/pylibcudf/libcudf/io/parquet.pxdpython/pylibcudf/tests/io/test_parquet.py
|
/merge |
Description
Follow up ##22955
This PR adds python bindings for Parquet column selection by field ID
Checklist