feat(phase2): Arrow memory model — Buffer, Bitmap, columnar arrays

[unikdahal]

Summary

• Implements Module 2.1 of design-docs/basalt-phase2-lld.md: Buffer, MutableBuffer, Bitmap/BitmapBuilder, and the Array trait with PrimitiveArray<T>/BooleanArray/StringArray + builders.
• Added alongside Phase 1's row-oriented Column/Value (untouched — still what exec/expr/io run on), not a replacement yet. The LLD's full migration map (retargeting execution, CSV, and the binder onto these types) is a separate, much larger cut-over; this PR is deliberately just the foundation, built completely and tested hard, since the LLD itself says everything else depends on getting it right.
• 168 tests passing (131 → 168), clippy clean under -D warnings, fmt applied.

Notable design decisions (deviating from the LLD in places, with reasoning)

• 64-byte alignment via an over-allocated Vec<u8> + computed align_offset, not a hand-rolled unsafe allocator — the LLD's own non-goal is "no unsafe unless measured," and a custom aligned allocator is exactly that.
• Bitmap and/or/not are bit-at-a-time, not word-at-a-time yet — correctness first at the LLD's own flagged highest-bug-density area; the SIMD fast path is a benchmarked follow-up.
• Invariants validated once at construction (try_new), not on every access — hot accessors (values(), value()) never need to return Result or panic. Builders bypass try_new via a private from_parts_unchecked since they produce trusted data themselves.
• StringBuilder only appends whole &strs, so offsets always land on UTF-8 boundaries by construction — value() uses from_utf8_unchecked justified by that plus one whole-buffer UTF-8 check in try_new.

Test plan

• cargo build
• cargo test — 168 passed, including the LLD-mandated bit-offset suite (1, 7, 8, 9, 63, 64, 65) and a zero-copy refcount proof for Buffer::slice
• cargo clippy --all-targets -- -D warnings
• cargo fmt --all -- --check

Summary by CodeRabbit

• New Features

  • Added a columnar data engine supporting nullable numbers, text, and booleans.
  • Added vectorized arithmetic, comparisons, filtering, casting, sorting, joins, aggregation, and row selection.
  • Added query planning and execution for projections, limits, scans, grouping, and joins.
  • Added Parquet reading, writing, schema discovery, projection, and row-group filtering.
  • Added zero-copy slicing and efficient batch operations.

• Documentation

  • Updated project documentation with architecture, build checks, benchmark instructions, and performance results.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces Phase 2 Arrow-native columnar execution with aligned shared buffers, typed arrays, vectorized compute kernels, logical and physical planning, batch execution operators including joins and aggregates, external sort infrastructure, Parquet I/O with pushdown, and comprehensive benchmarking comparing Phase 1 vs Phase 2 performance.

Changes

Phase 2 columnar execution foundation

Layer / File(s)	Summary
Shared buffers and bitmaps src/buffer/native.rs, src/buffer/bitmap.rs, src/buffer/buffer.rs, src/buffer/mutable.rs, src/buffer/mod.rs	Defines NativeType for safe byte reinterpretation, aligned immutable Buffer, mutable builders with alignment padding, bitmaps with boolean operators and bit-packed validity, and public buffer exports.
Array interface and type mapping src/array/array.rs, src/array/types.rs, src/array/mod.rs	Introduces the Array trait for type introspection and null handling, ArrayRef shared pointers, downcasting helpers (as_primitive, as_boolean, as_string), and Arrow primitive type markers (Int64Type, Float64Type).
Primitive, boolean, and string arrays src/array/primitive.rs, src/array/boolean.rs, src/array/string.rs	Implements fixed-width nullable primitives, bit-packed booleans, and variable-length UTF-8 strings with builders, validation, zero-copy slices, and test coverage for invariants.
Batch representation and scalars src/batch.rs, src/scalar.rs, src/types/schema.rs	Adds ColumnarBatch with schema validation and projection, ScalarValue for typed single values including nulls, and SchemaRef shared pointers with duplicate-name constructor for joins.

**Vectorized compute kernels**

Layer / File(s)	Summary
Compute dispatch and kernel entry points src/compute/mod.rs	Introduces ColumnarValue (array vs scalar union), compute module structure, and vectorized kernel exports.
Arithmetic operations src/compute/arith.rs	Implements add, sub, mul, div, rem with checked overflow, null propagation, scalar/array shapes, and branch-free overflow detection for Int64 operations with comprehensive null-skipping semantics.
Boolean and comparison kernels src/compute/boolean.rs, src/compute/comparison.rs, src/compute/cast.rs	Adds Kleene three-valued boolean logic (and_kleene, or_kleene, not_kleene), elementwise comparisons (eq, neq, lt, lteq, gt, gteq) with type enforcement and NaN handling, and vectorized type casting with validation.
Data movement kernels src/compute/filter.rs, src/compute/take.rs, src/compute/concat.rs, src/compute/sort.rs, src/compute/index.rs	Implements WHERE filtering (true-only semantics), gather (take with bounds checking and null propagation), concatenation, lexicographic sorting to indices, and UInt32Array index storage with builders.

**Logical and physical planning**

Layer / File(s)	Summary
Logical query plans src/logical_plan/plan.rs, src/logical_plan/builder.rs, src/logical_plan/display.rs, src/logical_plan/mod.rs	Introduces logical plan tree (TableScan, Filter, Projection, Aggregate, Join, Sort, Limit), fluent builder API with schema derivation per operation, and pretty-printing display for debugging.
Physical expressions src/physical_expr/expr.rs, src/physical_expr/column.rs, src/physical_expr/literal.rs, src/physical_expr/binary.rs, src/physical_expr/unary.rs, src/physical_expr/cast.rs, src/physical_expr/is_null.rs, src/physical_expr/mod.rs	Defines PhysicalExpr trait for batch-time evaluation, implements column references, literals, binary ops with coercion, unary ops (NOT, NEG), casts, and null checks with type/nullability inference.
Batch execution operators src/physical_plan/plan.rs, src/physical_plan/scan.rs, src/physical_plan/filter.rs, src/physical_plan/projection.rs, src/physical_plan/limit.rs, src/physical_plan/sort.rs, src/physical_plan/mod.rs	Implements ExecutionPlan trait and operators: in-memory scan, filter with predicate evaluation, projection with expression batching, offset/limit with cross-batch slicing, and full sort plus top-k with bounded heaps.
Join operators src/physical_plan/join/hash_join.rs, src/physical_plan/join/nested_loop.rs, src/physical_plan/join/mod.rs	Implements hash join (all join types with equi-keys and optional residual filters) and nested-loop join (non-equi predicates) with NULL key handling and output assembly per join type.
Aggregate operators src/physical_plan/aggregate/accumulator.rs, src/physical_plan/aggregate/group_keys.rs, src/physical_plan/aggregate/hash_table.rs, src/physical_plan/aggregate/mod.rs	Adds two-phase aggregation with accumulators (SUM, COUNT, MIN, MAX, AVG), group key encoding with order-preserving numeric mappings, hash-based grouping, and per-group state reduction.
External sort infrastructure src/physical_plan/spill.rs	Introduces memory reservations with atomic budgeting, batch serialization via group key encoding, and row-based I/O for external sorting.
Physical planning src/physical_plan/planner.rs	Implements logical-to-physical conversion for all plan node types, expression translation, and a Sort+Limit → TopKExec optimization.

**I/O and integration**

Layer / File(s)	Summary
Parquet I/O src/io/parquet.rs, src/io/mod.rs	Adds Parquet read/write with Arrow conversion, schema discovery, row-group predicate pushdown via statistics, and optional column projection.

**Benchmarking, testing, and documentation**

Layer / File(s)	Summary
Benchmark infrastructure Cargo.toml, benches/row_vs_batch.rs, benches/kernel_only.rs, benches/tpch.rs	Adds Criterion benchmarks comparing Phase 1 (row-at-a-time) vs Phase 2 (batch-at-a-time) for arithmetic, kernels, and end-to-end TPC-H Q6 queries across multiple input sizes.
Profiling examples examples/profile_add.rs	Provides example code for timing and profiling Basalt arithmetic operations with various approaches.
Documentation README.md, BENCHMARKS.md	Updates project status to claim Phase 1 and Phase 2 implemented, expands architectural layout and build/test commands, and documents measured performance results including four rounds of debugging and optimization.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 86.16% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main Phase 2 memory-model and columnar array additions.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch phase2/2.1-memory-model

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

src/buffer/buffer.rs (1)

96-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

slice() bounds checks are debug_assert!-only in both Buffer and Bitmap. Both zero-copy slice constructors validate offset + len <= self.len only via debug_assert!, so release builds silently build an invalid view whose failure only surfaces later as a less-diagnostic panic from indexing inside as_slice()/get().

• src/buffer/buffer.rs#L96-L103: promote the Buffer::slice bounds check to an unconditional check (e.g. assert! or return a Result) so misuse fails at the call site instead of downstream.
• src/buffer/bitmap.rs#L74-L87: apply the same unconditional bounds check to Bitmap::slice.

🤖 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 `@src/buffer/buffer.rs` around lines 96 - 103, Replace the debug-only bounds
validation in Buffer::slice with an unconditional check so invalid offset and
length values fail at the call site; preserve the existing zero-copy view
behavior for valid ranges. Apply the same unconditional bounds check in
Bitmap::slice; update src/buffer/buffer.rs:96-103 and
src/buffer/bitmap.rs:74-87.

🤖 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.

Inline comments:
In `@src/array/primitive.rs`:
- Around line 160-163: Replace the debug-only bounds checks with hard assertions
in the safe slice methods: change the assertion guarding PrimitiveArray::slice
in src/array/primitive.rs lines 160-163 and the corresponding BooleanArray slice
check in src/array/boolean.rs lines 74-77 from debug_assert! to assert!,
preserving their existing conditions and messages.
- Around line 22-26: Remove the offset field from PrimitiveArray and update all
constructors, slicing logic, and value accessors to rely on Buffer and Bitmap’s
internal offsets. In particular, adjust slice and values so they no longer read
or add self.offset, while preserving aligned validity/value slicing and existing
length behavior; update any struct initializers accordingly.

In `@src/array/string.rs`:
- Around line 49-50: Update the public StringArray::try_new constructor to
validate every provided offset before storing or using it: require offsets to be
within values' byte length, ordered as expected by value(), and positioned on
UTF-8 character boundaries. Return the existing error type for invalid offsets
so value() can safely retain its unchecked UTF-8 conversion without
out-of-bounds access or undefined behavior.
- Around line 147-151: Update append_value to calculate the prospective values
length and validate it converts to i32 before mutating self.values. Only after
this check succeeds should it extend self.values and push the corresponding
offset, preserving builder state unchanged when overflow occurs.

In `@src/buffer/buffer.rs`:
- Around line 21-26: Replace the derived Debug implementations for AlignedBytes
and Buffer with bounded custom implementations that expose only safe metadata,
never raw or padded byte contents. Ensure formatting Buffer directly, through
Arc<AlignedBytes>, or via Bitmap cannot dump payload data, while retaining
useful structural fields such as lengths and offsets where appropriate.

---

Nitpick comments:
In `@src/buffer/buffer.rs`:
- Around line 96-103: Replace the debug-only bounds validation in Buffer::slice
with an unconditional check so invalid offset and length values fail at the call
site; preserve the existing zero-copy view behavior for valid ranges. Apply the
same unconditional bounds check in Bitmap::slice; update
src/buffer/buffer.rs:96-103 and src/buffer/bitmap.rs:74-87.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 985b6dec-8193-495c-a4e9-e9ed43d5b806

📥 Commits

Reviewing files that changed from the base of the PR and between 4f1d689 and fe533d4.

📒 Files selected for processing (12)

• src/array/array.rs
• src/array/boolean.rs
• src/array/mod.rs
• src/array/primitive.rs
• src/array/string.rs
• src/array/types.rs
• src/buffer/bitmap.rs
• src/buffer/buffer.rs
• src/buffer/mod.rs
• src/buffer/mutable.rs
• src/buffer/native.rs
• src/lib.rs

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Remove redundant offset field to prevent double-offset bugs and simplify the struct.

Unlike upstream Arrow where ArrayData tracks the offset because its buffers are absolute, this codebase's Buffer and Bitmap natively track their own internal offsets (as seen in Buffer::slice and Bitmap::slice).

Currently, PrimitiveArray always sets offset: 0 during construction and slicing. If it were ever non-zero, it would introduce two critical bugs:

1. validity and values would become misaligned during slicing, because validity is sliced using just offset while values incorrectly attempts to use (self.offset + offset).
2. values() would panic with an out-of-bounds error, because the sliced Buffer is already sized down exactly to self.len, making &full[self.offset..] out of bounds.

Removing the offset field simplifies the type (making it consistent with BooleanArray) and removes this footgun.

♻️ Proposed refactor to remove the offset field

 pub struct PrimitiveArray<T: ArrowPrimitiveType> {
     values: Buffer,
     validity: Option<Bitmap>,
-    offset: usize,
     len: usize,

Apply these changes to the rest of the file to remove all usages of offset:

@@ -39,7 +38,6 @@ impl<T: ArrowPrimitiveType> Clone for PrimitiveArray<T> {
         fn clone(&self) -> Self {
             PrimitiveArray {
                 values: self.values.clone(),
                 validity: self.validity.clone(),
-                offset: self.offset,
                 len: self.len,
                 _phantom: PhantomData,
             }
         }
 
@@ -88,7 +86,6 @@     pub fn try_new(values: Buffer, validity: Option<Bitmap>) -> Result<Self> {
             Ok(PrimitiveArray {
                 values,
                 validity,
-                offset: 0,
                 len,
                 _phantom: PhantomData,
             })
         }
 
@@ -110,7 +107,6 @@     pub(crate) fn from_parts_unchecked(
             PrimitiveArray {
                 values,
                 validity,
-                offset: 0,
                 len,
                 _phantom: PhantomData,
             }
         }
 
@@ -122,8 +118,7 @@     pub fn values(&self) -> &[T::Native] {
             // SAFETY: validated once in `try_new` (or guaranteed by the
             // `from_parts_unchecked` caller contract); `Buffer` is immutable, so
             // the invariant holds for the array's whole lifetime.
-            let full = unsafe { self.values.typed_data_unchecked::<T::Native>() };
-            &full[self.offset..self.offset + self.len]
+            unsafe { self.values.typed_data_unchecked::<T::Native>() }
         }
 
@@ -165,10 +160,9 @@     fn slice(&self, offset: usize, len: usize) -> ArrayRef {
             Arc::new(PrimitiveArray::<T> {
                 values: self
                     .values
-                    .slice((self.offset + offset) * elem_size, len * elem_size),
+                    .slice(offset * elem_size, len * elem_size),
                 validity: self.validity.as_ref().map(|v| v.slice(offset, len)),
-                offset: 0,
                 len,
                 _phantom: PhantomData,
             })

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	pub struct PrimitiveArray<T: ArrowPrimitiveType> {
	values: Buffer,
	validity: Option<Bitmap>,
	offset: usize,
	len: usize,
	pub struct PrimitiveArray<T: ArrowPrimitiveType> {
	values: Buffer,
	validity: Option<Bitmap>,
	len: usize,

🤖 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 `@src/array/primitive.rs` around lines 22 - 26, Remove the offset field from
PrimitiveArray and update all constructors, slicing logic, and value accessors
to rely on Buffer and Bitmap’s internal offsets. In particular, adjust slice and
values so they no longer read or add self.offset, while preserving aligned
validity/value slicing and existing length behavior; update any struct
initializers accordingly.

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Enforce safe slicing bounds in release mode to prevent Undefined Behavior.

The safe slice methods use debug_assert! to validate bounds, which strips the check in release builds. This allows callers to pass out-of-bounds offset or len values. Because the underlying Buffer::slice and Bitmap::slice operations likely also use debug assertions (per provided Context snippets), this creates a returned struct holding an out-of-bounds length. When values() is later called on that array, it blindly trusts this length to construct a Rust slice (&[T]), leading to immediate Undefined Behavior (UB) when the bounds exceed the underlying allocation.

Replace debug_assert! with a hard assert! to guarantee memory safety.

• src/array/primitive.rs#L160-L163: Change debug_assert! to assert!.
• src/array/boolean.rs#L74-L77: Change debug_assert! to assert!.

📍 Affects 2 files

• src/array/primitive.rs#L160-L163 (this comment)
• src/array/boolean.rs#L74-L77

🤖 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 `@src/array/primitive.rs` around lines 160 - 163, Replace the debug-only bounds
checks with hard assertions in the safe slice methods: change the assertion
guarding PrimitiveArray::slice in src/array/primitive.rs lines 160-163 and the
corresponding BooleanArray slice check in src/array/boolean.rs lines 74-77 from
debug_assert! to assert!, preserving their existing conditions and messages.

[coderabbitai]

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Validate offsets against bounds and UTF-8 boundaries to ensure memory safety.

try_new is a public constructor, but it currently fails to validate that the provided offsets are within the bounds of values and land on valid UTF-8 character boundaries. Because value(i) relies on these invariants and uses unsafe { std::str::from_utf8_unchecked(...) }, invalid offsets passed to try_new can cause out-of-bounds panics or undefined behavior (UB) in safe code.

🛡️ Proposed fix to ensure memory safety

-        std::str::from_utf8(values.as_slice())
-            .map_err(|e| BasaltError::Internal(format!("values buffer is not valid UTF-8: {e}")))?;
+        let valid_str = std::str::from_utf8(values.as_slice())
+            .map_err(|e| BasaltError::Internal(format!("values buffer is not valid UTF-8: {e}")))?;
+
+        for &offset in offsets_typed {
+            if offset < 0 || !valid_str.is_char_boundary(offset as usize) {
+                return Err(BasaltError::Internal(format!(
+                    "offset {} is out of bounds or not a char boundary",
+                    offset
+                )));
+            }
+        }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	std::str::from_utf8(values.as_slice())
	.map_err(|e| BasaltError::Internal(format!("values buffer is not valid UTF-8: {e}")))?;
	let valid_str = std::str::from_utf8(values.as_slice())
	.map_err(|e| BasaltError::Internal(format!("values buffer is not valid UTF-8: {e}")))?;
	for &offset in offsets_typed {
	if offset < 0 || !valid_str.is_char_boundary(offset as usize) {
	return Err(BasaltError::Internal(format!(
	"offset {} is out of bounds or not a char boundary",
	offset
	)));
	}
	}

🤖 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 `@src/array/string.rs` around lines 49 - 50, Update the public
StringArray::try_new constructor to validate every provided offset before
storing or using it: require offsets to be within values' byte length, ordered
as expected by value(), and positioned on UTF-8 character boundaries. Return the
existing error type for invalid offsets so value() can safely retain its
unchecked UTF-8 conversion without out-of-bounds access or undefined behavior.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Prevent corrupted builder state on overflow.

If append_value fails because the new total length exceeds i32::MAX, self.values has already been mutated. This leaves the builder in an unrecoverable, inconsistent state where the internal values buffer is out of sync with offsets and len, corrupting any subsequent appends or the final array.

Calculate and check the new length before mutating self.values.

🔒️ Proposed fix to ensure atomic updates

     pub fn append_value(&mut self, s: &str) -> Result<()> {
-        self.values.extend_from_slice(s.as_bytes());
-        let end = i32::try_from(self.values.len())
+        let new_len = self.values.len() + s.len();
+        let end = i32::try_from(new_len)
             .map_err(|_| BasaltError::Internal("StringArray offsets overflowed i32".to_string()))?;
+        self.values.extend_from_slice(s.as_bytes());
         self.offsets.push(end);
         self.validity.push(true);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	pub fn append_value(&mut self, s: &str) -> Result<()> {
	self.values.extend_from_slice(s.as_bytes());
	let end = i32::try_from(self.values.len())
	.map_err(|_| BasaltError::Internal("StringArray offsets overflowed i32".to_string()))?;
	self.offsets.push(end);
	pub fn append_value(&mut self, s: &str) -> Result<()> {
	let new_len = self.values.len() + s.len();
	let end = i32::try_from(new_len)
	.map_err(|_| BasaltError::Internal("StringArray offsets overflowed i32".to_string()))?;
	self.values.extend_from_slice(s.as_bytes());
	self.offsets.push(end);

🤖 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 `@src/array/string.rs` around lines 147 - 151, Update append_value to calculate
the prospective values length and validate it converts to i32 before mutating
self.values. Only after this check succeeds should it extend self.values and
push the corresponding offset, preserving builder state unchanged when overflow
occurs.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Derived Debug on AlignedBytes/Buffer dumps raw bytes — PII/logging risk.

#[derive(Debug)] on AlignedBytes prints the entire raw: Vec<u8> (including the over-allocated padding), and Buffer's derived Debug delegates through Arc<AlignedBytes> to the same. Since Buffer is the backing store for arbitrary column bytes — including string/text data that may contain real customer content — any future {:?}/panic message/log statement touching a Buffer (directly, or via a Bitmap, which also holds one) will dump raw payload data. Notably, PrimitiveArray's hand-written Debug impl (src/array/primitive.rs) deliberately avoids printing values for exactly this reason — the convention this file should also follow.

🔒 Proposed fix: bounded custom Debug impl

-#[derive(Debug)]
 struct AlignedBytes {
     raw: Vec<u8>,
     align_offset: usize,
     len: usize,
 }
+
+impl std::fmt::Debug for AlignedBytes {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("AlignedBytes")
+            .field("len", &self.len)
+            .field("align_offset", &self.align_offset)
+            .finish()
+    }
+}

-#[derive(Clone, Debug)]
+#[derive(Clone)]
 pub struct Buffer {
     data: Arc<AlignedBytes>,
     offset: usize,
     len: usize,
 }
+
+impl std::fmt::Debug for Buffer {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Buffer")
+            .field("offset", &self.offset)
+            .field("len", &self.len)
+            .finish()
+    }
+}

Also applies to: 57-62

🤖 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 `@src/buffer/buffer.rs` around lines 21 - 26, Replace the derived Debug
implementations for AlignedBytes and Buffer with bounded custom implementations
that expose only safe metadata, never raw or padded byte contents. Ensure
formatting Buffer directly, through Arc<AlignedBytes>, or via Bitmap cannot dump
payload data, while retaining useful structural fields such as lengths and
offsets where appropriate.

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/types/schema.rs (1)

83-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Allow duplicate field names in projected schemas.

Schema::project uses Schema::new(fields) to construct the resulting schema, which strictly enforces unique field names. If a projection is applied over a joined schema that legitimately contains duplicate names (explicitly permitted via new_allow_duplicate_names), or if a query deliberately duplicates a column in its output (e.g., SELECT a, a FROM t), this will return an error and crash the query planning or execution.

Use Schema::new_allow_duplicate_names instead to preserve the source schema's tolerance for duplicates.

🐛 Proposed fix

     /// Build a new schema from a subset of columns, in the given order. For projection.
     pub fn project(&self, indices: &[usize]) -> Result<Schema> {
         let fields = indices
             .iter()
             .map(|&i| {
                 self.field(i).cloned().ok_or_else(|| BasaltError::Schema {
                     message: format!("field index {i} out of bounds"),
                 })
             })
             .collect::<Result<Vec<_>>>()?;
-        Schema::new(fields)
+        Ok(Schema::new_allow_duplicate_names(fields))
     }

🤖 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 `@src/types/schema.rs` around lines 83 - 94, Update Schema::project to
construct projected schemas with Schema::new_allow_duplicate_names instead of
Schema::new, preserving duplicate field names from joined schemas or repeated
projections while keeping the existing field-index validation unchanged.

🧹 Nitpick comments (7)

src/physical_plan/aggregate/mod.rs (1)

126-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate strict scalars_to_array helper. src/physical_plan/aggregate/mod.rs and src/physical_plan/spill.rs carry byte-for-byte identical strict scalars_to_array implementations (same match arms, same error messages); consider hoisting one copy into a shared module (e.g. crate::array or a small scalar_arrays util) so the two stay in lock-step as new DataTypes are added. Note the src/physical_plan/sort.rs copy is intentionally lenient (_ => append_null) and should stay separate.

• src/physical_plan/aggregate/mod.rs#L126-L189: replace with a call to the shared helper.
• src/physical_plan/spill.rs#L167-L231: replace with a call to the shared helper.

🤖 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 `@src/physical_plan/aggregate/mod.rs` around lines 126 - 189, The strict
scalars_to_array implementation is duplicated across both aggregate and spill
paths. Move one strict implementation into a shared array/scalar utility, then
replace the helpers in src/physical_plan/aggregate/mod.rs:126-189 and
src/physical_plan/spill.rs:167-231 with calls to it; leave the lenient
scalars_to_array in src/physical_plan/sort.rs unchanged.

src/physical_expr/unary.rs (1)

112-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicated Int64/Float64 negation loops.

The Array branch of NegExpr::evaluate repeats the same is_null/append_null/append_value loop for Int64 and Float64, differing only in the value type and negation op (checked vs. plain). Could be factored into a small generic helper once more numeric types are added, but not urgent with only two variants today.

🤖 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 `@src/physical_expr/unary.rs` around lines 112 - 148, Keep the current Int64
and Float64 negation implementations unchanged; the duplicated loops are an
optional future refactor, not a required fix. If refactoring now, extract the
shared null-preserving array-negation logic from NegExpr::evaluate while
retaining checked negation for Int64 and plain negation for Float64.

README.md (3)

39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add language specifier to fenced code block.

As indicated by static analysis hints, specifying a language enhances rendering and syntax highlighting. Use sh for these shell commands.

♻️ Proposed fix

-```
+```sh
 cargo build
 cargo test
 cargo clippy --all-targets -- -D warnings
 cargo fmt --all

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @README.md around lines 39 - 44, Update the fenced shell-command block in the
README to specify the sh language identifier, while preserving all existing
commands and formatting.

</details>

<!-- cr-comment:v1:e8e63246c6e412da19b97e46 -->

_Source: Linters/SAST tools_

---

`48-50`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_

**Add language specifier to fenced code block.**

As indicated by static analysis hints, specifying a language enhances rendering and syntax highlighting. Use `sh` for these shell commands.





<details>
<summary>♻️ Proposed fix</summary>

```diff
-```
+```sh
 cargo bench --bench row_vs_batch

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @README.md around lines 48 - 50, Update the fenced code block containing the
cargo bench command to specify the sh language while preserving the command and
its formatting.

</details>

<!-- cr-comment:v1:b1fd8fcd7551f76c828ad892 -->

_Source: Linters/SAST tools_

---

`14-31`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_

**Add language specifier to fenced code block.**

As indicated by static analysis hints, specifying a language enhances rendering and syntax highlighting. Use `text` for this directory tree block.





<details>
<summary>♻️ Proposed fix</summary>

```diff
-```
+```text
 src/
 ├── types/         # DataType, Value, Schema, coercion
 ├── array/         # Buffer, Bitmap, Array trait, PrimitiveArray/BooleanArray/StringArray

🤖 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 `@README.md` around lines 14 - 31, Update the fenced directory-tree block in
the README to specify the text language, changing the opening fence to use text
while preserving the tree contents and formatting.

Source: Linters/SAST tools

src/physical_plan/aggregate/hash_table.rs (1)

90-106: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Hoist rows_per_group out of the per-aggregate loop.

rows_per_group depends only on group_of_row and the group count, both invariant across agg_arrays. Rebuilding it (a fresh Vec<Vec<u32>> allocation plus a full re-scan of group_of_row) once per aggregate multiplies the grouping cost by the number of aggregates on the aggregation hot path.

♻️ Compute grouping once, reuse across aggregates

-        for (agg_idx, agg_array) in agg_arrays.iter().enumerate() {
-            let mut rows_per_group: Vec<Vec<u32>> = vec![Vec::new(); self.group_keys.len()];
-            for (row, &g) in group_of_row.iter().enumerate() {
-                rows_per_group[g].push(row as u32);
-            }
-            for (g, rows) in rows_per_group.into_iter().enumerate() {
-                if rows.is_empty() {
-                    continue; // group existed before this batch but got no rows in it
-                }
-                let mut idx_builder = UInt32Builder::with_capacity(rows.len());
-                for r in rows {
-                    idx_builder.append_value(r);
-                }
-                let gathered = take(agg_array.as_ref(), &idx_builder.finish())?;
-                self.accumulators[agg_idx][g].update_batch(&[gathered])?;
-            }
-        }
+        let mut rows_per_group: Vec<Vec<u32>> = vec![Vec::new(); self.group_keys.len()];
+        for (row, &g) in group_of_row.iter().enumerate() {
+            rows_per_group[g].push(row as u32);
+        }
+        for (agg_idx, agg_array) in agg_arrays.iter().enumerate() {
+            for (g, rows) in rows_per_group.iter().enumerate() {
+                if rows.is_empty() {
+                    continue; // group existed before this batch but got no rows in it
+                }
+                let mut idx_builder = UInt32Builder::with_capacity(rows.len());
+                for &r in rows {
+                    idx_builder.append_value(r);
+                }
+                let gathered = take(agg_array.as_ref(), &idx_builder.finish())?;
+                self.accumulators[agg_idx][g].update_batch(&[gathered])?;
+            }
+        }

🤖 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 `@src/physical_plan/aggregate/hash_table.rs` around lines 90 - 106, Hoist the
construction of rows_per_group out of the per-aggregate loop in the aggregate
update logic. Build it once from group_of_row and self.group_keys.len(), then
reuse it while iterating over agg_arrays; preserve the existing empty-group
handling, take gathering, and accumulator updates.

src/io/parquet.rs (1)

108-211: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Pre-size the UTF-8 builder. StringBuilder::with_capacity(a.len(), 0) under-reserves the string payload and can force extra reallocations; use a.value_data().len() (the Arrow string buffer size) instead. The primitive branches don’t expose a direct bulk-copy path in the current Basalt array APIs, so the string branch is the clear low-cost win.

🤖 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 `@src/io/parquet.rs` around lines 108 - 211, Update the UTF-8 branch in
arrow_array_to_basalt to initialize StringBuilder with the Arrow string buffer
size from a.value_data().len() as its payload capacity, while preserving the
existing element-count capacity and null/value handling.

🤖 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.

Inline comments:
In `@benches/tpch.rs`:
- Around line 113-118: Update the vector construction containing quantity,
extendedprice, discount, and shipdate so its elements are cast with as _ to the
expected ArrayRef trait-object type. Ensure the first element,
quantity.finish(), does not force a concrete Int64 array type, allowing all
heterogeneous arrays to compile as Vec<ArrayRef>.

In `@src/batch.rs`:
- Around line 357-365: Update ColumnarBatch::slice to validate that offset + len
does not exceed self.num_rows before slicing columns or constructing the result.
Fail fast on invalid ranges, including zero-column batches, while preserving the
existing zero-copy behavior for valid slices.

In `@src/compute/cast.rs`:
- Around line 41-52: Update the Float64-to-Int64 conversion in the (Float64,
Int64) branch to require v to be strictly less than i64::MAX as f64, while
retaining the existing finite and lower-bound checks and error behavior.

In `@src/io/parquet.rs`:
- Around line 229-259: Update row_group_may_match so Int64 statistics and
predicate values remain in exact i64 arithmetic, rather than converting them to
f64. Split the Int64 and Double/Float64 handling into type-specific comparison
paths, preserving the existing min/max fallback and unsupported-type behavior;
only the Double path should use f64 comparisons.

In `@src/logical_plan/builder.rs`:
- Around line 60-107: Update expr_display_name and its aggregate() call site to
accept the group-expression position and disambiguate non-Column names,
preserving column names while generating distinct names for repeated expression
outputs. Change the group_expr iteration in aggregate() to pass the enumerated
index, and update any other callers of expr_display_name consistently.

---

Outside diff comments:
In `@src/types/schema.rs`:
- Around line 83-94: Update Schema::project to construct projected schemas with
Schema::new_allow_duplicate_names instead of Schema::new, preserving duplicate
field names from joined schemas or repeated projections while keeping the
existing field-index validation unchanged.

---

Nitpick comments:
In `@README.md`:
- Around line 39-44: Update the fenced shell-command block in the README to
specify the sh language identifier, while preserving all existing commands and
formatting.
- Around line 48-50: Update the fenced code block containing the cargo bench
command to specify the sh language while preserving the command and its
formatting.
- Around line 14-31: Update the fenced directory-tree block in the README to
specify the text language, changing the opening fence to use text while
preserving the tree contents and formatting.

In `@src/io/parquet.rs`:
- Around line 108-211: Update the UTF-8 branch in arrow_array_to_basalt to
initialize StringBuilder with the Arrow string buffer size from
a.value_data().len() as its payload capacity, while preserving the existing
element-count capacity and null/value handling.

In `@src/physical_expr/unary.rs`:
- Around line 112-148: Keep the current Int64 and Float64 negation
implementations unchanged; the duplicated loops are an optional future refactor,
not a required fix. If refactoring now, extract the shared null-preserving
array-negation logic from NegExpr::evaluate while retaining checked negation for
Int64 and plain negation for Float64.

In `@src/physical_plan/aggregate/hash_table.rs`:
- Around line 90-106: Hoist the construction of rows_per_group out of the
per-aggregate loop in the aggregate update logic. Build it once from
group_of_row and self.group_keys.len(), then reuse it while iterating over
agg_arrays; preserve the existing empty-group handling, take gathering, and
accumulator updates.

In `@src/physical_plan/aggregate/mod.rs`:
- Around line 126-189: The strict scalars_to_array implementation is duplicated
across both aggregate and spill paths. Move one strict implementation into a
shared array/scalar utility, then replace the helpers in
src/physical_plan/aggregate/mod.rs:126-189 and
src/physical_plan/spill.rs:167-231 with calls to it; leave the lenient
scalars_to_array in src/physical_plan/sort.rs unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d10aa126-88ec-477d-bd46-cd99fb5ecd14

📥 Commits

Reviewing files that changed from the base of the PR and between fe533d4 and 596bd36.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (62)

• BENCHMARKS.md
• Cargo.toml
• README.md
• benches/kernel_only.rs
• benches/row_vs_batch.rs
• benches/tpch.rs
• examples/profile_add.rs
• src/array/array.rs
• src/array/boolean.rs
• src/array/mod.rs
• src/array/primitive.rs
• src/array/string.rs
• src/array/types.rs
• src/batch.rs
• src/buffer/bitmap.rs
• src/buffer/buffer.rs
• src/buffer/mod.rs
• src/buffer/mutable.rs
• src/buffer/native.rs
• src/compute/arith.rs
• src/compute/boolean.rs
• src/compute/cast.rs
• src/compute/comparison.rs
• src/compute/concat.rs
• src/compute/filter.rs
• src/compute/index.rs
• src/compute/mod.rs
• src/compute/sort.rs
• src/compute/take.rs
• src/io/mod.rs
• src/io/parquet.rs
• src/lib.rs
• src/logical_plan/builder.rs
• src/logical_plan/display.rs
• src/logical_plan/mod.rs
• src/logical_plan/plan.rs
• src/physical_expr/binary.rs
• src/physical_expr/cast.rs
• src/physical_expr/column.rs
• src/physical_expr/expr.rs
• src/physical_expr/is_null.rs
• src/physical_expr/literal.rs
• src/physical_expr/mod.rs
• src/physical_expr/unary.rs
• src/physical_plan/aggregate/accumulator.rs
• src/physical_plan/aggregate/group_keys.rs
• src/physical_plan/aggregate/hash_table.rs
• src/physical_plan/aggregate/mod.rs
• src/physical_plan/filter.rs
• src/physical_plan/join/hash_join.rs
• src/physical_plan/join/mod.rs
• src/physical_plan/join/nested_loop.rs
• src/physical_plan/limit.rs
• src/physical_plan/mod.rs
• src/physical_plan/plan.rs
• src/physical_plan/planner.rs
• src/physical_plan/projection.rs
• src/physical_plan/scan.rs
• src/physical_plan/sort.rs
• src/physical_plan/spill.rs
• src/scalar.rs
• src/types/schema.rs

🚧 Files skipped from review as they are similar to previous changes (11)

• src/buffer/mod.rs
• src/buffer/native.rs
• src/array/types.rs
• src/array/mod.rs
• src/buffer/mutable.rs
• src/buffer/bitmap.rs
• src/buffer/buffer.rs
• src/array/primitive.rs
• src/array/string.rs
• src/array/boolean.rs
• src/array/array.rs

[coderabbitai]

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Fix type inference mismatch in vec!.

The vec! macro determines the element type from the first element. Because quantity.finish() returns a PrimitiveArray<Int64Type> and extendedprice.finish() returns a PrimitiveArray<Float64Type>, they are different concrete types, which will cause a compilation error (expected struct PrimitiveArray<Int64Type>, found struct PrimitiveArray<Float64Type>).

Cast the elements (or at least the first one) to the expected trait object type using as _ to allow the compiler to successfully infer Vec<ArrayRef> (or Vec<Arc<dyn Array>>).

🐛 Proposed fix for explicit type casting

         vec![
-            Arc::new(quantity.finish()),
-            Arc::new(extendedprice.finish()),
-            Arc::new(discount.finish()),
-            Arc::new(shipdate.finish()),
+            Arc::new(quantity.finish()) as _,
+            Arc::new(extendedprice.finish()) as _,
+            Arc::new(discount.finish()) as _,
+            Arc::new(shipdate.finish()) as _,
         ],

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	vec![
	Arc::new(quantity.finish()),
	Arc::new(extendedprice.finish()),
	Arc::new(discount.finish()),
	Arc::new(shipdate.finish()),
	],
	vec![
	Arc::new(quantity.finish()) as _,
	Arc::new(extendedprice.finish()) as _,
	Arc::new(discount.finish()) as _,
	Arc::new(shipdate.finish()) as _,
	],

🤖 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 `@benches/tpch.rs` around lines 113 - 118, Update the vector construction
containing quantity, extendedprice, discount, and shipdate so its elements are
cast with as _ to the expected ArrayRef trait-object type. Ensure the first
element, quantity.finish(), does not force a concrete Int64 array type, allowing
all heterogeneous arrays to compile as Vec<ArrayRef>.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add bounds checking to slice.

slice does not verify that offset + len <= self.num_rows. While the underlying non-empty arrays might panic on out-of-bounds slicing, a 0-column batch will silently succeed and illegally inflate its num_rows. Adding an explicit bounds check enforces the batch invariant and fails fast.

🛡️ Proposed fix

     /// O(1): every column's `slice` is a zero-copy `Arc`-shared window.
     pub fn slice(&self, offset: usize, len: usize) -> ColumnarBatch {
+        assert!(
+            offset + len <= self.num_rows,
+            "slice offset and length exceed batch row count"
+        );
         let columns = self.columns.iter().map(|c| c.slice(offset, len)).collect();
         ColumnarBatch {
             schema: self.schema.clone(),
             columns,
             num_rows: len,
         }
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/// O(1): every column's `slice` is a zero-copy `Arc`-shared window.
	pub fn slice(&self, offset: usize, len: usize) -> ColumnarBatch {
	let columns = self.columns.iter().map(|c| c.slice(offset, len)).collect();
	ColumnarBatch {
	schema: self.schema.clone(),
	columns,
	num_rows: len,
	}
	}
	/// O(1): every column's `slice` is a zero-copy `Arc`-shared window.
	pub fn slice(&self, offset: usize, len: usize) -> ColumnarBatch {
	assert!(
	offset + len <= self.num_rows,
	"slice offset and length exceed batch row count"
	);
	let columns = self.columns.iter().map(|c| c.slice(offset, len)).collect();
	ColumnarBatch {
	schema: self.schema.clone(),
	columns,
	num_rows: len,
	}
	}

🤖 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 `@src/batch.rs` around lines 357 - 365, Update ColumnarBatch::slice to validate
that offset + len does not exceed self.num_rows before slicing columns or
constructing the result. Fail fast on invalid ranges, including zero-column
batches, while preserving the existing zero-copy behavior for valid slices.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Rust f64 as i64 cast saturating behavior and i64::MAX rounding to f64

💡 Result:

In Rust, the as operator performs a saturating cast when converting a floating-point type (like f64) to an integer type (like i64) [1][2]. This behavior is well-defined [3]. The conversion rules for f64 to i64 using as are as follows: 1. Rounding: The fractional part of the f64 value is discarded, effectively rounding towards zero [1][2]. 2. Saturation: If the value exceeds the range of the destination integer type (i64), it is clamped to the type's bounds [3][2]. - Values greater than i64::MAX (including f64::INFINITY) saturate to i64::MAX [3][4]. - Values less than i64::MIN (including f64::NEG_INFINITY) saturate to i64::MIN [3][4]. 3. NaN handling: NaN (Not-a-Number) values are converted to 0 [3][4]. Regarding the conversion of i64::MAX to f64: - The type f64 uses an IEEE 754 double-precision floating-point format, which has a 53-bit significand. Consequently, it cannot exactly represent all 64-bit integers [1][2]. - The constant i64::MAX (2^63 - 1) is larger than the maximum integer exactly representable in f64 [2][5]. When you cast i64::MAX to f64, the value is rounded to the nearest representable f64 value [1][2]. Because of this loss of precision, casting i64::MAX to f64 and then back to i64 will not yield the original i64::MAX value [2]. If you require precise rounding or need to handle out-of-range values explicitly, it is recommended to use methods such as f64::round (to control the rounding strategy before casting) or to utilize the TryFrom trait for checked conversions, which can return an error if the value is out of range, rather than relying on the silent saturation of the as operator [1][6][7].

Citations:

• 1: https://learnrust.net/chapter-4/numeric-conversions/
• 2: https://rust-for-c-programmers.com/ch16/16_2_primitive_casting_with_as.html
• 3: Define UB in float-to-int casts to saturate rust-lang/rust#71269
• 4: https://rust.googlesource.com/rust/+/0ce5963923c8809036302d927ee6545cdd2071f9/tests/ui/numbers-arithmetic/saturating-float-casts-impl.rs
• 5: https://doc.rust-lang.org/std/primitive.f64.html
• 6: https://stackoverflow.com/questions/19607564/how-does-rust-handle-rounding-in-conversion-of-f64-to-i64
• 7: https://stackoverflow.com/questions/41138599/rounding-a-f64-to-nearest-i64-in-rust

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '--- src/compute/cast.rs (relevant excerpt) ---\n'
nl -ba src/compute/cast.rs | sed -n '1,90p'

printf '\n--- boundary probe (Rust semantics via Python float + reasoning) ---\n'
python3 - <<'PY'
import math
vals = [
    float(2**63 - 1),
    float(2**63),
    float(2**63 + 1),
    float(-(2**63)),
    float(-(2**63) - 1),
]
for v in vals:
    print(repr(v), 'finite=', math.isfinite(v), '>=min=', v >= float(-(2**63)), '<=max=', v <= float(2**63 - 1), '<2^63=', v < float(2**63 - 1))
PY

Repository: unikdahal/basalt

Length of output: 245

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- src/compute/cast.rs (relevant excerpt) ---'
nl -ba src/compute/cast.rs | sed -n '35,60p'

echo
echo '--- float boundary probe ---'
python3 - <<'PY'
import math

max_i64 = 2**63 - 1
min_i64 = -(2**63)

vals = [
    max_i64,
    max_i64 + 1,
    2**63,
    min_i64,
    min_i64 - 1,
]

for n in vals:
    v = float(n)
    print(
        f"n={n} v={v!r} finite={math.isfinite(v)} "
        f">=min={v >= float(min_i64)} <=max={v <= float(max_i64)} <2^63={v < float(2**63)}"
    )

print("\nfloat(i64::MAX) == float(2**63):", float(max_i64) == float(2**63))
print("float(i64::MAX):", float(max_i64))
print("float(2**63):", float(2**63))
PY

Repository: unikdahal/basalt

Length of output: 241

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- src/compute/cast.rs (relevant excerpt) ---'
cat -n src/compute/cast.rs | sed -n '35,60p'

echo
echo '--- float boundary probe ---'
python3 - <<'PY'
import math

min_i64 = -(2**63)
max_i64 = 2**63 - 1

for n in [max_i64, max_i64 + 1, 2**63, min_i64, min_i64 - 1]:
    v = float(n)
    print(
        f"n={n} v={v!r} finite={math.isfinite(v)} "
        f">=min={v >= float(min_i64)} <=max={v <= float(max_i64)} <2^63={v < float(2**63)}"
    )

print("float(i64::MAX) == float(2**63):", float(max_i64) == float(2**63))
print("float(i64::MAX):", float(max_i64))
print("float(2**63):", float(2**63))
PY

Repository: unikdahal/basalt

Length of output: 2029

---

Upper bound should be strict

i64::MAX as f64 rounds to 2^63, so v <= i64::MAX as f64 lets 2^63 through and v as i64 saturates to i64::MAX. Use < here.

🤖 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 `@src/compute/cast.rs` around lines 41 - 52, Update the Float64-to-Int64
conversion in the (Float64, Int64) branch to require v to be strictly less than
i64::MAX as f64, while retaining the existing finite and lower-bound checks and
error behavior.

[coderabbitai]

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Float conversion loses precision for large Int64 statistics, defeating the function's own "never wrongly skip" guarantee.

(min as f64, max as f64) and ScalarValue::Int64(Some(v)) => v as f64 round every i64 beyond 2^53 (~9.007e15) to the nearest representable f64. Nanosecond-epoch timestamps and large surrogate/sequence IDs regularly exceed this threshold. Once rounded, a Gt/Lt/GtEq/LtEq comparison that is true on the real integers can become false (or vice versa) purely from rounding, causing a row group to be wrongly skipped — exactly the "silently wrong answer" failure mode the doc comment above (Line 227) says this function must never produce.

Keep Int64 statistics comparisons in exact i64 arithmetic; only convert to f64 for the Double/Float64 branch.

🐛 Proposed fix: compare Int64 statistics exactly, without float conversion

 fn row_group_may_match(stats: Option<&Statistics>, pred: &RowGroupPredicate) -> bool {
     let Some(stats) = stats else { return true };
 
-    let (min, max): (f64, f64) = match (stats, &pred.value) {
-        (Statistics::Int64(s), ScalarValue::Int64(Some(_))) => match (s.min_opt(), s.max_opt()) {
-            (Some(&min), Some(&max)) => (min as f64, max as f64),
-            _ => return true,
-        },
-        (Statistics::Double(s), ScalarValue::Float64(Some(_))) => {
-            match (s.min_opt(), s.max_opt()) {
-                (Some(&min), Some(&max)) => (min, max),
-                _ => return true,
-            }
-        }
-        _ => return true, // Utf8/Boolean statistics, or a type mismatch: not supported here.
-    };
-    let value = match pred.value {
-        ScalarValue::Int64(Some(v)) => v as f64,
-        ScalarValue::Float64(Some(v)) => v,
-        _ => return true,
-    };
-
-    match pred.op {
-        BinaryOp::Gt => max > value,
-        BinaryOp::GtEq => max >= value,
-        BinaryOp::Lt => min < value,
-        BinaryOp::LtEq => min <= value,
-        BinaryOp::Eq => min <= value && value <= max,
-        _ => true, // NotEq and non-comparison ops: statistics can't prove exclusion.
-    }
+    match (stats, &pred.value) {
+        (Statistics::Int64(s), ScalarValue::Int64(Some(value))) => {
+            let Some((&min, &max)) = s.min_opt().zip(s.max_opt()) else { return true };
+            match pred.op {
+                BinaryOp::Gt => max > *value,
+                BinaryOp::GtEq => max >= *value,
+                BinaryOp::Lt => min < *value,
+                BinaryOp::LtEq => min <= *value,
+                BinaryOp::Eq => min <= *value && *value <= max,
+                _ => true,
+            }
+        }
+        (Statistics::Double(s), ScalarValue::Float64(Some(value))) => {
+            let Some((&min, &max)) = s.min_opt().zip(s.max_opt()) else { return true };
+            match pred.op {
+                BinaryOp::Gt => max > *value,
+                BinaryOp::GtEq => max >= *value,
+                BinaryOp::Lt => min < *value,
+                BinaryOp::LtEq => min <= *value,
+                BinaryOp::Eq => min <= *value && *value <= max,
+                _ => true,
+            }
+        }
+        _ => true, // Utf8/Boolean statistics, or a type mismatch: not supported here.
+    }
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	fn row_group_may_match(stats: Option<&Statistics>, pred: &RowGroupPredicate) -> bool {
	let Some(stats) = stats else { return true };
	let (min, max): (f64, f64) = match (stats, &pred.value) {
	(Statistics::Int64(s), ScalarValue::Int64(Some(_))) => match (s.min_opt(), s.max_opt()) {
	(Some(&min), Some(&max)) => (min as f64, max as f64),
	_ => return true,
	},
	(Statistics::Double(s), ScalarValue::Float64(Some(_))) => {
	match (s.min_opt(), s.max_opt()) {
	(Some(&min), Some(&max)) => (min, max),
	_ => return true,
	}
	}
	_ => return true, // Utf8/Boolean statistics, or a type mismatch: not supported here.
	};
	let value = match pred.value {
	ScalarValue::Int64(Some(v)) => v as f64,
	ScalarValue::Float64(Some(v)) => v,
	_ => return true,
	};
	match pred.op {
	BinaryOp::Gt => max > value,
	BinaryOp::GtEq => max >= value,
	BinaryOp::Lt => min < value,
	BinaryOp::LtEq => min <= value,
	BinaryOp::Eq => min <= value && value <= max,
	_ => true, // NotEq and non-comparison ops: statistics can't prove exclusion.
	}
	}
	fn row_group_may_match(stats: Option<&Statistics>, pred: &RowGroupPredicate) -> bool {
	let Some(stats) = stats else { return true };
	match (stats, &pred.value) {
	(Statistics::Int64(s), ScalarValue::Int64(Some(value))) => {
	let Some((&min, &max)) = s.min_opt().zip(s.max_opt()) else { return true };
	match pred.op {
	BinaryOp::Gt => max > *value,
	BinaryOp::GtEq => max >= *value,
	BinaryOp::Lt => min < *value,
	BinaryOp::LtEq => min <= *value,
	BinaryOp::Eq => min <= *value && *value <= max,
	_ => true,
	}
	}
	(Statistics::Double(s), ScalarValue::Float64(Some(value))) => {
	let Some((&min, &max)) = s.min_opt().zip(s.max_opt()) else { return true };
	match pred.op {
	BinaryOp::Gt => max > *value,
	BinaryOp::GtEq => max >= *value,
	BinaryOp::Lt => min < *value,
	BinaryOp::LtEq => min <= *value,
	BinaryOp::Eq => min <= *value && *value <= max,
	_ => true,
	}
	}
	_ => true, // Utf8/Boolean statistics, or a type mismatch: not supported here.
	}
	}

🤖 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 `@src/io/parquet.rs` around lines 229 - 259, Update row_group_may_match so
Int64 statistics and predicate values remain in exact i64 arithmetic, rather
than converting them to f64. Split the Int64 and Double/Float64 handling into
type-specific comparison paths, preserving the existing min/max fallback and
unsupported-type behavior; only the Double path should use f64 comparisons.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

expr_display_name collapses to "expr" for any non-Column expression, causing name collisions on multi-key aggregates.

aggregate() names each group_expr field via expr_display_name, which only special-cases Expr::Column; every other expression variant returns the literal "expr" (Line 169). A GROUP BY with two or more non-column expressions (e.g. GROUP BY a + 1, b + 1) produces two fields both named "expr", and Schema::new(fields) — which this same file's join() doc comment/tests confirm rejects duplicate names — will error out on construction.

🔧 Proposed fix: disambiguate by position

-fn expr_display_name(expr: &Expr) -> String {
-    match expr {
-        Expr::Column { index, .. } => format!("col_{index}"),
-        _ => "expr".to_string(),
-    }
-}
+fn expr_display_name(expr: &Expr, position: usize) -> String {
+    match expr {
+        Expr::Column { index, .. } => format!("col_{index}"),
+        _ => format!("expr_{position}"),
+    }
+}

And update the call site to pass the loop index: for (i, expr) in group_expr.iter().enumerate() { fields.push(Field::new(expr_display_name(expr, i), ...)); }

Also applies to: 166-171

🤖 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 `@src/logical_plan/builder.rs` around lines 60 - 107, Update expr_display_name
and its aggregate() call site to accept the group-expression position and
disambiguate non-Column names, preserving column names while generating distinct
names for repeated expression outputs. Change the group_expr iteration in
aggregate() to pass the enumerated index, and update any other callers of
expr_display_name consistently.