[Data] Fix error message and off-by-one bounds check in operators#64576
[Data] Fix error message and off-by-one bounds check in operators#64576jiangxt2 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses three bugs: it corrects an assertion error message in _concatenate_chunked_arrays to display the actual tensor type instead of None, and fixes off-by-one bounds checking errors in both UnionOperator and ZipOperator input addition methods. It also introduces corresponding unit tests. The feedback suggests refactoring the _concatenate_chunked_arrays test to use the more idiomatic pytest.raises context manager instead of a try-except block with pytest.fail.
Fix three minor bugs in Ray Data operators:
1. _concatenate_chunked_arrays prints "type: None" instead of the actual
array type in its assertion error message. Changed {type_} to {arr.type}.
2. UnionOperator._add_input_inner uses <= instead of < in bounds check,
allowing out-of-bounds index access under python -O.
3. ZipOperator._add_input_inner has the same off-by-one bounds check.
Both off-by-one bugs are corrected to match the pattern used by
MixOperator._add_input_inner.
Signed-off-by: jiangxt2 <jiangxt2@vip.qq.com>
13dcbd7 to
f227ae9
Compare
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
Why are these changes needed?
Three minor bugs in Ray Data operators:
_concatenate_chunked_arraysprintstype: Nonein its assertion error message instead of the actual array type. The f-string uses{type_}which is alwaysNoneat that point in the loop — it is initialized toNoneand only reassigned after the assertion.UnionOperator._add_input_inneruses<=instead of<in its bounds check, allowinginput_index == len(self._input_dependencies). This is semantically incorrect —input_indexis 0-based, so valid values are[0, len-1]. The<=allows an out-of-bounds index to pass the assertion.ZipOperator._add_input_innerhas the same off-by-one bounds check.Both off-by-one bugs are corrected to match the pattern used by
MixOperator._add_input_inner(line 95), which correctly uses strict<.What changes are made?
python/ray/data/_internal/arrow_ops/transform_pyarrow.py: Change{type_}to{arr.type}in assertion error messagepython/ray/data/_internal/execution/operators/union_operator.py: Change<=to<in bounds checkpython/ray/data/_internal/execution/operators/zip_operator.py: Change<=to<in bounds checkpython/ray/data/tests/unit/test_fix_operator_bounds_check.py: Add 3 tests covering the fixesRelated issues
N/A
Checks
git commit -s)