Deprecate windowslinetermination CSV option - #23363
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe CSV reader deprecates Windows line termination APIs, adds CRLF parsing coverage, and removes the deprecated option from the benchmark configuration. ChangesCSV line termination handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change deprecates an unused CSV option while preserving the default CRLF-compatible behavior and adding regression coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
Could a maintainer please add the improvement and non-breaking labels? This PR introduces a non-breaking API deprecation, and Label Checker is currently failing. |
|
I pushed a fix in commit ecf0463, please review it. |
|
@coderabbitai I pushed a fix in commit 1f54e05, please review it. |
|
✅ Action performedReview finished.
|
|
@coderabbitai I pushed a fix in commit f532729, please review it |
|
✅ Action performedReview finished.
|
|
/ok to test 3f55fe0 |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/ok to test 09473b4 |
|
I triggered CI again. Please don't merge the upstream branch again if CI passes. :) |
|
/ok to test 572ab2d |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/ok to test e090d1f |
|
/ok to test bef5971 |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@utkarshparekh, the new test is failing in CI |
The new CRLF test parsed the values correctly, but it used CUDF_TEST_EXPECT_TABLES_EQUAL, which also requires identical nullability. The CSV reader output is nullable with zero nulls, while the expected wrappers are non-nullable. I’ve updated the new test to use CUDF_TEST_EXPECT_TABLES_EQUIVALENT, so it verifies the intended values and types without requiring identical null-mask representation. Could you please approve the new commit for CI? |
|
/ok to test f9e78b5 |
|
The new CRLF test now passes across the full conda-cpp-tests matrix. The remaining spark-rapids-jni failure is the known downstream RMM compatibility issue tracked in NVIDIA/cudf-spark-jni#5041, with a fix in NVIDIA/cudf-spark-jni#5042. No additional CSV-related change appears necessary in this PR. |
|
The remaining pre-commit.ci failure is unrelated to this PR’s CSV changes. It comes from python/cudf/cudf/core/udf/rolling_utils.py, introduced on main by #23598, and reports numpydoc errors GL08 and RT01. The CSV-related hooks and C++ test matrix pass. I have not modified the unrelated Python file in this PR. |
Description
Deprecates the unused libcudf
windowslineterminationCSV reader option. The option is stored incsv_reader_optionsbut is not used by the CSV reader.This change:
lineterminator='\n', which supports CRLF input.No Python-facing deprecation warning is needed because this option is not exposed through the Python API.
Closes #15985.
Testing
CsvReaderTest.WindowsLineTerminators.Checklist