Skip to content

feat: add chunk row accessors for typed and string reads#129

Open
mrchypark wants to merge 7 commits into
duckdb:mainfrom
mrchypark:feature/upstream-query-chunks-accessors
Open

feat: add chunk row accessors for typed and string reads#129
mrchypark wants to merge 7 commits into
duckdb:mainfrom
mrchypark:feature/upstream-query-chunks-accessors

Conversation

@mrchypark

@mrchypark mrchypark commented Apr 3, 2026

Copy link
Copy Markdown

Summary

  • add ChunkRows plus Conn.QueryChunksContext and Stmt.QueryChunksContext for chunk-by-chunk result access
  • add typed DataChunk accessors for INTEGER, BIGINT, DOUBLE, TIMESTAMP, and zero-copy VARCHAR reads
  • add tests and benchmarks covering chunk iteration, null handling, type mismatches, and chunk-oriented performance

Motivation

The existing database/sql row path is still the right default, but some consumers need a lower-overhead read path when they already process results chunk-wise. This change keeps the existing API untouched and adds an opt-in chunk API on top of the existing DataChunk abstraction.

The typed accessors are intentionally narrow:

  • IsNull
  • Int32Slice
  • Int64Slice
  • Float64Slice
  • TimestampMicrosSlice
  • StringRefs

All returned views are documented as valid only until the next NextChunk call or Close.

Benchmarks

On an Apple M1 with:

go test ./... -run '^$' -bench 'BenchmarkQueryChunksContext(TypedNumericScan|GetValueNumericScan|TypedStringScan|Count)$' -benchmem -benchtime=1x
  • BenchmarkQueryChunksContextGetValueNumericScan: 37.57 ms/op, 1.06 MB/op, 131162 allocs/op
  • BenchmarkQueryChunksContextTypedNumericScan: 4.66 ms/op, 13.9 KB/op, 346 allocs/op
  • BenchmarkQueryChunksContextTypedStringScan: 13.51 ms/op, 1.07 MB/op, 444 allocs/op
  • BenchmarkQueryChunksContextCount: 4.71 ms/op, 18.1 KB/op, 411 allocs/op

The main improvement is removing per-cell boxing and GetValue overhead when the caller can already consume chunk-oriented data.

Validation

  • go test ./...
  • go test ./... -run '^$' -bench 'BenchmarkQueryChunksContext(TypedNumericScan|GetValueNumericScan|TypedStringScan|Count)$' -benchmem -benchtime=1x

@mlafeldt

Copy link
Copy Markdown
Member

I would appreciate any guidance on how you would like this to be shaped before a full review starts.

Thanks for the draft. I think the overall shape looks good. Keeping it as an explicit low-level, opt-in path is the right call. Please keep it focused. We can add more typed accessors later. Maybe add a test for empty and non-inlined strings as well.

@mlafeldt mlafeldt added the feature / enhancement Code improvements or a new feature label Apr 21, 2026
@mrchypark

Copy link
Copy Markdown
Author

I would appreciate any guidance on how you would like this to be shaped before a full review starts.

Thanks for the draft. I think the overall shape looks good. Keeping it as an explicit low-level, opt-in path is the right call. Please keep it focused. We can add more typed accessors later. Maybe add a test for empty and non-inlined strings as well.

Thanks, that makes sense. I'll keep this PR focused and add coverage for empty strings and non-inlined strings in the StringRefs tests.

@mrchypark
mrchypark marked this pull request as ready for review April 21, 2026 09:51
@taniabogatsch
taniabogatsch requested a review from mlafeldt May 28, 2026 13:59

@mlafeldt mlafeldt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. A few things to fix before merge:

  1. StringRefs crashes on NULL VARCHAR rows
  2. IsNull doesn't validate rowIdx
  3. TimestampMicrosSlice doc is inaccurate
  4. CI fails: golangci-lint run reports 8 issues

See my inline comments.

Comment thread data_chunk_typed.go Outdated
Comment thread data_chunk_typed.go
Comment thread data_chunk_string.go
@mlafeldt mlafeldt added the changes requested Changes have been requested to a PR or issue label Jun 10, 2026
@mrchypark
mrchypark requested a review from mlafeldt June 20, 2026 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Changes have been requested to a PR or issue feature / enhancement Code improvements or a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants