python/src/datagen.rs-32-53 (1)

32-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Include sizing inputs in both error paths.

The exceptions omit the requested row/byte sizes and batch count, making invalid configuration failures difficult to diagnose. Include parameter names and values in the conflicting-options and reader-construction errors.

As per coding guidelines, **/*.{rs,py,java} requires full error context, including “variable names, values, sizes, and types.”

🤖 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/src/datagen.rs` around lines 32 - 53, Update the validation error in
the rows_in_batch/bytes_in_batch conflict check and the reader-construction
error inside the match on rows_in_batch to include the relevant parameter names
and values: rows_in_batch, bytes_in_batch, and batch_count, including resolved
defaults where applicable. Preserve the existing exception types and generation
behavior while providing full sizing context in both messages.

Source: Coding guidelines

python/python/ci_benchmarks/benchmarks/test_overlay_read.py-36-38 (1)

36-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Parameterize the return type.

Use list[int] instead of bare list so the indices passed to ds.take retain their element type.

-def _take_indices(num_rows: int) -> list:
+def _take_indices(num_rows: int) -> list[int]:

As per coding guidelines, python/**/*.py requires parameterized type hints and prohibits bare generics.

🤖 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/python/ci_benchmarks/benchmarks/test_overlay_read.py` around lines 36
- 38, Update the _take_indices function’s return annotation from the bare list
generic to list[int], preserving the existing sorted random-index behavior and
ensuring the indices passed to ds.take retain their element type.

Source: Coding guidelines

python/python/ci_benchmarks/overlays.py-70-74 (1)

70-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate dimensions before the modulo operation.

rows_per_file=0 raises ZeroDivisionError, and num_rows=0 proceeds until later helpers assume a first fragment exists. Reject non-positive dimensions with a contextual ValueError before calculating divisibility.

Proposed fix

 def make_base_dataset(
 ...
 ) -> lance.LanceDataset:
+    if (
+        not isinstance(num_rows, int)
+        or isinstance(num_rows, bool)
+        or not isinstance(rows_per_file, int)
+        or isinstance(rows_per_file, bool)
+        or num_rows <= 0
+        or rows_per_file <= 0
+    ):
+        raise ValueError(
+            "num_rows and rows_per_file must be positive integers; "
+            f"got num_rows={num_rows!r} ({type(num_rows).__name__}), "
+            f"rows_per_file={rows_per_file!r} ({type(rows_per_file).__name__})"
+        )
     if num_rows % rows_per_file:

As per coding guidelines, **/*.{rs,py,java} requires validating API-boundary inputs with descriptive errors.

🤖 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/python/ci_benchmarks/overlays.py` around lines 70 - 74, Validate
num_rows and rows_per_file as positive values before the divisibility check in
the surrounding overlay setup. Raise a contextual ValueError for either
non-positive dimension, then retain the existing modulo-based multiple
validation for valid inputs.

Source: Coding guidelines

python/python/ci_benchmarks/overlays.py-95-100 (1)

95-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject invalid coverage and preserve the stride pattern.

max(1, ...) silently turns zero or negative coverage into one cell. Also, when count > num_rows / 2, step becomes 1, so "stride" degenerates into a leading contiguous run rather than distributing coverage across the fragment.

Proposed fix

+import math
+
 def coverage_offsets(num_rows: int, fraction: float, pattern: str) -> List[int]:
 ...
-    count = max(1, int(round(num_rows * fraction)))
+    if num_rows <= 0 or not math.isfinite(fraction) or not 0 < fraction <= 1:
+        raise ValueError(
+            f"expected num_rows > 0 and 0 < fraction <= 1; "
+            f"got num_rows={num_rows!r}, fraction={fraction!r}"
+        )
+    count = int(round(num_rows * fraction))
+    if count == 0:
+        raise ValueError(
+            f"fraction={fraction!r} selects zero rows for num_rows={num_rows!r}"
+        )
     if pattern == "contiguous":
         return list(range(count))
     if pattern == "stride":
-        step = max(1, num_rows // count)
-        return list(range(0, num_rows, step))[:count]
+        return [(index * num_rows) // count for index in range(count)]

As per coding guidelines, **/*.{rs,py,java} requires invalid inputs to be rejected and never silently adjusted.

🤖 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/python/ci_benchmarks/overlays.py` around lines 95 - 100, Update the
coverage/count calculation in the overlay selection logic to reject zero or
negative coverage instead of clamping it to one, raising the module’s
appropriate invalid-input exception. Revise the “stride” branch so selected rows
remain distributed across the full range when count exceeds half of num_rows,
rather than degenerating into a leading contiguous run; preserve existing
contiguous behavior.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes