Skip to content

Add Iterator support in clickhouse-go#1821

Open
prnvbn wants to merge 3 commits intoClickHouse:mainfrom
prnvbn:main
Open

Add Iterator support in clickhouse-go#1821
prnvbn wants to merge 3 commits intoClickHouse:mainfrom
prnvbn:main

Conversation

@prnvbn
Copy link
Copy Markdown

@prnvbn prnvbn commented Apr 7, 2026

Summary

#1820

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added

@github-actions
Copy link
Copy Markdown

Summary

This PR introduces StructIter[T] — a Go 1.23 range-over-function iterator that wraps driver.Rows and yields (T, error) pairs using struct scanning. The approach is sound: defer rows.Close() ensures the connection is always released, and the rows.Err() check after the loop correctly surfaces network-level errors. Since the project's go.mod requires Go 1.24.1, the iter package is fully supported with no compatibility concern.


Should fix

1. rows.Close() error is silently dropped

The Close() method returns an error. CLAUDE.md policy is explicit: never swallow errors. A close failure (e.g. a network flush error) will be invisible to the caller. One approach: capture the close error and yield it if no earlier error was delivered:

return func(yield func(T, error) bool) {
    var closeErr error
    defer func() { closeErr = rows.Close() }()

    for rows.Next() {
        var value T
        if err := rows.ScanStruct(&value); err != nil {
            var zero T
            yield(zero, err)
            return
        }
        if !yield(value, nil) {
            return
        }
    }

    if err := rows.Err(); err != nil {
        var zero T
        yield(zero, err)
        return
    }
    if closeErr != nil {
        var zero T
        yield(zero, closeErr)
    }
}

2. StructIter has no doc comment

Every exported symbol must have a full-sentence doc comment beginning with the symbol name. Currently the function has none. At minimum:

// StructIter returns a range-over-function iterator that scans each row from rows
// into a value of type T using ScanStruct, closing rows when iteration ends.
// The second yield value is non-nil if scanning fails or rows.Err() is set.
// T must be a struct. StructIter is available only via the native clickhouse.Open() API;
// database/sql.Rows does not implement driver.Rows.
func StructIter[T any](rows Rows) iter.Seq2[T, error] {

3. No integration test in tests/ — HTTP protocol is untested

The only non-unit test is TestIterators in examples/clickhouse_api/, which uses the native TCP path. The checklist requires coverage for both TCP and HTTP protocol cases. Please add a test under tests/ that exercises StructIter over a real ClickHouse instance on both transports. The unit tests in lib/driver/iter_test.go mock Rows, which is appropriate for testing iterator control flow in isolation, but they do not replace protocol-level coverage.

4. std API path not addressed

StructIter accepts driver.Rows, which is only obtained from the native clickhouse.Open() API. database/sql.Rows does not implement driver.Rows, so the iterator is silently unusable with the std path. The checklist requires coverage (or at minimum documentation) of both APIs. Either add a std-compatible variant or add a clear note to the doc comment and README explaining the limitation.


Nits

  • nit: defer conn.Exec(ctx, "DROP TABLE example_iterators") in the example discards an error. Assign to a named return error or log it, consistent with the rest of the function.
  • nit: The _ = yield(zero, err) lines on error paths are intentional (we return immediately after, so the bool is irrelevant), but a short inline comment — // stop regardless of caller preference — would make the intent explicit to readers unfamiliar with the range-over-function protocol.

Verdict

Request changes — the missing doc comment and silent swallowing of rows.Close() errors are both clear policy violations per CLAUDE.md. The gap in HTTP/std protocol test coverage is also required by the review checklist before merging.

@prnvbn
Copy link
Copy Markdown
Author

prnvbn commented May 4, 2026

addressed the comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants