⚡ Bolt: Reduces bulk scan execution time by ~10%

[cungminh2710]

💡 What: Optimized row scanning logic for "direct" types (integers, strings, booleans, floats, and time.Time) by using field offsets and unsafe.Pointer assignments instead of reflection-based Set* methods.

🎯 Why: Reflection-based assignments in the hot scanning loop (executed per row per column) were a measurable bottleneck for large result sets.

📊 Impact: Reduces bulk scan execution time by ~8-10% for medium-sized datasets.

🔬 Measurement: Verified with BenchmarkSQLiteSelectBulkScan/medium (~3.1ms -> ~2.8ms). All integration tests passed.

---

PR created automatically by Jules for task 18071894035170893412 started by @cungminh2710

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[greptile-apps]

Greptile Summary

This PR optimizes the hot per-row scan loop in scanDirectRowAddr by pre-computing struct field offsets at plan-build time and using unsafe.Pointer arithmetic to write directly to memory instead of going through reflection's Set* methods for integer, string, boolean, float, and time.Time direct fields. Fields accessed through embedded pointer intermediaries fall back to the existing reflection path via a canUseOffset flag.

• scanColumnPlan gains three fields (offset, kind, canUseOffset) populated in newRowScanPlanForColumns; the hot path in scanDirectRowAddr uses these to bypass reflection for non-complex fields.
• The Float32 overflow check has been updated to correctly match reflect.OverflowFloat semantics, allowing ±Inf and NaN to pass through while returning an error only for finite overflow.
• Non-complex pointer columns (int64Pointer, stringPointer, etc.) continue to use fieldByIndexAlloc unchanged, since they require nil-check and allocation logic that already dominates their cost.

Confidence Score: 4/5

Safe to merge for the common case; two maintainability concerns around the float switch and GC write barriers are worth addressing but won't cause failures with current callers.

The core optimization is sound: offset computation is correct, canUseOffset correctly gates all complex/pointer-embedded fields back to the reflection path, and the previously flagged Float32 overflow and ±Inf issues are properly fixed. The float64 fast-path switch silently skips assignment on an unexpected col.kind rather than falling back like the int path does, and unsafe writes to string and time.Time fields bypass GC write barriers for their internal pointer members. Neither causes a failure today, but both represent fragile patterns that warrant a follow-up.

pkg/rain/model.go — specifically the float64ValueCols canUseOffset switch and the string/time.Time unsafe write sites.

Important Files Changed

Filename	Overview
pkg/rain/model.go	Introduces unsafe-pointer fast path for int/string/bool/float/time.Time direct column scans; canUseOffset flag and per-plan field offsets computed at plan-build time; Float32 overflow and ±Inf handling fixed from previous review; two P2 observations remain (missing default in float switch, write-barrier bypass for string and time.Time).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[scanRowsAgainstTableDirect] --> B{pointerElems?}
    B -- yes --> C[reflect.New struct
scanDirectRow item.Elem]
    B -- no --> D[item.Set zeroElem
scanDirectRowAddr item.Addr.UnsafePointer]
    C --> E[scanDirectRow
computes baseAddr via Addr.UnsafePointer
calls scanDirectRowAddr]
    D --> F[scanDirectRowAddr]
    E --> F
    F --> G{col.canUseOffset?}
    G -- yes --> H[unsafe.Add baseAddr + col.offset
direct typed write]
    G -- no --> I[fieldByIndexAlloc
reflection Set*]
    H --> J{col.kind}
    J -- Int*/Uint* --> K[overflow check
then *int* ptr = val]
    J -- String --> L[*string ptr = v.String]
    J -- Bool --> M[*bool ptr = v.Bool]
    J -- Float32/64 --> N[overflow check for Float32
*float* ptr = val]
    J -- time.Time --> O[*time.Time ptr = v.Time]
    J -- default int only --> P[fieldByIndexAlloc fallback
assignRawValueToField]

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
pkg/rain/model.go:584-599
**Float64 fast-path switch has no `default` fallback**

The `switch col.kind` in the `canUseOffset` block for `float64ValueCols` handles `Float64` and `Float32` but has no `default` case. If `col.kind` were neither (which can't happen today, but could after a future change to `isSimpleDirectType`), the loop hits `continue` without writing anything to the field — silently leaving the destination at its zero value with no error. The equivalent int64 fast-path switch at line 365–429 has a proper `default:` block that falls back to `fieldByIndexAlloc` + `assignRawValueToField`, so the float path is inconsistently defensible against this class of silent failure.

### Issue 2 of 2
pkg/rain/model.go:494-496
**Unsafe writes for pointer-containing types bypass GC write barriers**

`*(*string)(unsafe.Add(baseAddr, col.offset)) = v.String` and `*(*time.Time)(unsafe.Add(baseAddr, col.offset)) = v.Time` (line 648) write structs that contain internal pointers (`string` carries a `*byte` data pointer; `time.Time` carries a `*time.Location`) through `unsafe.Pointer`, bypassing the hybrid write barrier Go's concurrent GC relies on. In the current usage the source values are always live on the stack so the pointed-to objects won't be collected, but the pattern violates the invariant that all pointer stores into heap memory must go through the write barrier. If the surrounding code ever changes (e.g. scratch lives in a sync.Pool on the heap), this could silently break GC correctness. Using `reflect.Value.SetString` / the existing `typedmemmove`-backed reflect path for these two types, or at minimum `//go:nowritebarrier`-aware runtime helpers, would make the intent explicit.

_{Reviews (5): Last reviewed commit: "refactor(rain): optimize row scanning wi..." | Re-trigger Greptile}

[cungminh2710]

@jules Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

Issue 1 of 3

pkg/rain/model.go:572-578
Float32 overflow check absent in fast path

When canUseOffset is true and col.kind == reflect.Float32, the code does a bare float32(v.Float64) with no overflow check. A float64 value like 1e40 would silently become +Inf in the stored field. The fallback (non-canUseOffset) path at line 586–590 correctly calls field.OverflowFloat(v.Float64) and returns an error, so the fast path now silently corrupts data where the old path would have errored.

Issue 2 of 3

pkg/rain/model.go:436
fieldByIndexAlloc errors silently dropped

Throughout scanDirectRowAddr, every call on the non-canUseOffset fallback path now uses field, _ := fieldByIndexAlloc(...), discarding the error. For complex fields whose canUseOffset is false (i.e. fields accessed through an embedded pointer intermediary), fieldByIndexAlloc can return a zero reflect.Value{} and a non-nil error when the intermediate pointer field is not settable. The caller then immediately invokes methods like field.Kind() or field.SetInt() on the zero value, which panics. The original code propagated this error cleanly; this PR regresses that behaviour.

Issue 3 of 3

pkg/rain/model.go:282-284
Dead code in elemSize calculation

elemSize is only ever consumed in the else branch (non-pointer elements, line 316). The if pointerElems { elemSize = target.Type().Elem().Size() } branch overwrites it with the pointer-element size, but that branch never uses elemSize—it calls scanDirectRow via reflection instead. The default initialisation elemSize := structType.Size() is already the correct value for the only path that reads it.

[google-labs-jules]

@jules Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

Issue 1 of 3

pkg/rain/model.go:572-578
Float32 overflow check absent in fast path

When canUseOffset is true and col.kind == reflect.Float32, the code does a bare float32(v.Float64) with no overflow check. A float64 value like 1e40 would silently become +Inf in the stored field. The fallback (non-canUseOffset) path at line 586–590 correctly calls field.OverflowFloat(v.Float64) and returns an error, so the fast path now silently corrupts data where the old path would have errored.

Issue 2 of 3

pkg/rain/model.go:436
fieldByIndexAlloc errors silently dropped

Throughout scanDirectRowAddr, every call on the non-canUseOffset fallback path now uses field, _ := fieldByIndexAlloc(...), discarding the error. For complex fields whose canUseOffset is false (i.e. fields accessed through an embedded pointer intermediary), fieldByIndexAlloc can return a zero reflect.Value{} and a non-nil error when the intermediate pointer field is not settable. The caller then immediately invokes methods like field.Kind() or field.SetInt() on the zero value, which panics. The original code propagated this error cleanly; this PR regresses that behaviour.

Issue 3 of 3

pkg/rain/model.go:282-284
Dead code in elemSize calculation

elemSize is only ever consumed in the else branch (non-pointer elements, line 316). The if pointerElems { elemSize = target.Type().Elem().Size() } branch overwrites it with the pointer-element size, but that branch never uses elemSize—it calls scanDirectRow via reflection instead. The default initialisation elemSize := structType.Size() is already the correct value for the only path that reads it.

I've addressed all three issues:

1. Float32 overflow check: Added an explicit check against math.MaxFloat32 in the canUseOffset path of scanDirectRowAddr.
2. fieldByIndexAlloc errors: Updated all fallback paths to check and return the error from fieldByIndexAlloc instead of ignoring it.
3. Dead code in elemSize: Removed the redundant branch that recalculated elemSize for pointer elements, as it is only consumed by the non-pointer path.

[cungminh2710]

@jules Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

Issue 1 of 2

pkg/rain/model.go:852-863
When canUseOffset is set to false mid-traversal (because an intermediate field is a pointer), the loop still continues to accumulate offset. The accumulated value is the sum of offsets before and after the pointer boundary, which does not correspond to any meaningful memory address relative to baseAddr. Although this value is never read when canUseOffset == false, storing a semantically invalid offset in the plan struct is likely to confuse future readers or maintainers who add new fast-path cases. Breaking out of the offset accumulation once canUseOffset is cleared would make the invariant explicit.

		if isComplex {
			fieldType = modelType
			for _, i := range fieldInfo.index {
				if fieldType.Kind() == reflect.Pointer {
					fieldType = fieldType.Elem()
					canUseOffset = false
				}
				f := fieldType.Field(i)
				if canUseOffset {
					offset += f.Offset
				}
				fieldType = f.Type
			}
		} else {

Issue 2 of 2

pkg/rain/model.go:584-605
The default branch in the float64ValueCols switch is unreachable: columns are added to float64ValueCols only for reflect.Float32 and reflect.Float64 kinds, and both are covered by the explicit cases above. The dead branch can be removed entirely to avoid the extra fieldByIndexAlloc call that would never execute.

		if col.canUseOffset {
			ptr := unsafe.Add(baseAddr, col.offset)
			switch col.kind {
			case reflect.Float64:
				*(*float64)(ptr) = v.Float64
			case reflect.Float32:
				f64 := v.Float64
				if f64 < -math.MaxFloat32 || f64 > math.MaxFloat32 {
					return fmt.Errorf("rain: value %f overflows float32", v.Float64)
				}
				*(*float32)(ptr) = float32(v.Float64)
			}
			continue
		}

[google-labs-jules]

@jules Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

Issue 1 of 2

pkg/rain/model.go:852-863
When canUseOffset is set to false mid-traversal (because an intermediate field is a pointer), the loop still continues to accumulate offset. The accumulated value is the sum of offsets before and after the pointer boundary, which does not correspond to any meaningful memory address relative to baseAddr. Although this value is never read when canUseOffset == false, storing a semantically invalid offset in the plan struct is likely to confuse future readers or maintainers who add new fast-path cases. Breaking out of the offset accumulation once canUseOffset is cleared would make the invariant explicit.

		if isComplex {
			fieldType = modelType
			for _, i := range fieldInfo.index {
				if fieldType.Kind() == reflect.Pointer {
					fieldType = fieldType.Elem()
					canUseOffset = false
				}
				f := fieldType.Field(i)
				if canUseOffset {
					offset += f.Offset
				}
				fieldType = f.Type
			}
		} else {

Issue 2 of 2

pkg/rain/model.go:584-605
The default branch in the float64ValueCols switch is unreachable: columns are added to float64ValueCols only for reflect.Float32 and reflect.Float64 kinds, and both are covered by the explicit cases above. The dead branch can be removed entirely to avoid the extra fieldByIndexAlloc call that would never execute.

		if col.canUseOffset {
			ptr := unsafe.Add(baseAddr, col.offset)
			switch col.kind {
			case reflect.Float64:
				*(*float64)(ptr) = v.Float64
			case reflect.Float32:
				f64 := v.Float64
				if f64 < -math.MaxFloat32 || f64 > math.MaxFloat32 {
					return fmt.Errorf("rain: value %f overflows float32", v.Float64)
				}
				*(*float32)(ptr) = float32(v.Float64)
			}
			continue
		}

I've implemented the suggested fixes for both issues:

1. Offset accumulation: The loop in newRowScanPlanForColumns now only increments the offset while canUseOffset remains true. This ensures the stored offset is always valid or zero.
2. Unreachable branch: Removed the redundant default branch in the float64ValueCols switch, as it was indeed unreachable.

[greptile-apps]

Float32 fast path incorrectly rejects ±Inf

The guard f64 > math.MaxFloat32 evaluates to true for +Inf, causing the fast path to return an error for infinity values. Go's reflect.Value.OverflowFloat uses the formula MaxFloat32 < |x| && |x| <= MaxFloat64, which evaluates to false for ±Inf (because ±Inf <= MaxFloat64 is false), so the slow path correctly accepts them and stores float32 ±Inf. Databases like PostgreSQL and SQLite can store ±Inf in float columns; any such value mapped to a float32 field will now error only when canUseOffset is true, creating an inconsistent failure depending on whether the field is embedded through a pointer.

The fix is to exempt infinities: !math.IsInf(f64, 0) && (f64 < -math.MaxFloat32 || f64 > math.MaxFloat32).

Prompt To Fix With AI

This is a comment left during a code review.
Path: pkg/rain/model.go
Line: 589-594

Comment:
**Float32 fast path incorrectly rejects ±Inf**

The guard `f64 > math.MaxFloat32` evaluates to `true` for `+Inf`, causing the fast path to return an error for infinity values. Go's `reflect.Value.OverflowFloat` uses the formula `MaxFloat32 < |x| && |x| <= MaxFloat64`, which evaluates to `false` for ±Inf (because `±Inf <= MaxFloat64` is false), so the slow path correctly accepts them and stores float32 ±Inf. Databases like PostgreSQL and SQLite can store ±Inf in float columns; any such value mapped to a float32 field will now error only when `canUseOffset` is true, creating an inconsistent failure depending on whether the field is embedded through a pointer.

The fix is to exempt infinities: `!math.IsInf(f64, 0) && (f64 < -math.MaxFloat32 || f64 > math.MaxFloat32)`.

How can I resolve this? If you propose a fix, please make it concise.

[cungminh2710]

@jules fix

[google-labs-jules]

I've updated the Float32 overflow check to exactly match the logic in reflect.OverflowFloat, which allows infinities and NaNs while erroring on true overflows. I've also improved the error message to include the field type for better context, consistent with other types.