[vuanhphung]

Skip the redundant Thrift FetchResults when direct results explicitly report hasMoreRows=false. Unknown or true flags still fetch normally. For complete CloudFetch and Arrow direct results, this matches Python, JDBC, and Node.

Preserve existing cleanup and error behavior: failed best-effort cleanup after a complete read leaves sql.Rows.Err() clear, while explicit early close still reports its error. Paginated results and Arrow iterator ownership retain their existing behavior.

For large_results_0005mb, timed RPCs change from ExecuteStatement → FetchResults → CloseOperation to ExecuteStatement → CloseOperation.

Client time in milliseconds against main at 95d97b3: medians from four measured pairs after warm-up, subtracting server duration. The paired delta is the median of within-round differences.

Dashboard case	Base	Fix	Paired delta
large_results_0005mb	768.9	683.6	-111.9
large_results_0010mb	846.9	821.7	-88.5
large_results_0025mb	961.0	977.7	-33.6
large_results_0050mb	1201.0	1139.5	-32.3
large_results_0100mb	1781.0	1592.1	-217.5
extended_types_array_inline	192.5	192.6	+0.4
low_latency_select_1	86.6	90.7	+4.3
low_latency_limit_0	91.3	85.5	-3.4

Validation: go test -race ./... and repository lint pass. Public database/sql coverage verifies unchanged EOF, early-close, and fetch-error behavior with successful/failing cleanup. Fresh live checksums match on all eight cases above and the separate 1 GB case; two 1 GB timing rounds return 6,491,176 rows for both versions and retain the needed page fetches.

[peco-review-bot]

Verdict: 1 Low

Looks good, with one low-severity API-shape concern. The change is correct: direct-result HasMoreRows=false now skips the extra FetchResults, and rows.Close() with closeOnce closes the operation and runs the telemetry callback exactly once. One related gap is outside this diff: in the GetArrowBatches/IPC path, the iterator's release() still issues CloseOperation itself and drops its error, so in that path Rows.Close() telemetry reports about 0 ms latency and no close error.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low concern. Honoring HasMoreRows=false and moving the operation close into a once-only Rows.Close() is correct for database/sql callers, and the new lifecycle and error tests cover that path well. The concern is that GetArrowBatches/GetArrowIPCStreams still close the operation early and drop the error, so close telemetry on those paths is wrong.

[peco-review-bot]

Verdict: 1 Low

Looks good overall, with one low-severity concern. Honoring a direct-results HasMoreRows=false and moving operation cleanup into a sync.Once-guarded rows.Close() is correct, and the new lifecycle and error-reporting tests are thorough. The remaining concern is that the IPC-stream iterator path no longer closes the operation when it is drained.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Looks good overall. Honoring HasMoreRows=false from direct results and moving cleanup to Rows.Close() is correct and well tested for row and IPC iteration. One medium concern: the GetArrowBatches iterator still closes the operation itself and drops the error, which defeats the close-error and telemetry handling this PR adds.

[peco-review-bot]

Verdict: 1 Medium · 1 Low

Mostly sound, with one medium gap: the GetArrowBatches record iterator still closes the operation itself, which bypasses the new Rows.Close() error reporting and telemetry. The sync.Once close, the hasMoreRows short-circuit and the test de-flaking look correct.

[peco-review-bot]

Verdict: 1 Low

Looks good: one low concern about close errors now showing up in sql.Rows.Err() after a clean read. The lifecycle refactor looks correct: closeOnce makes repeated Close() and the telemetry callback idempotent, GetArrowBatches still closes from its own iterator, the HasMoreRows=false short-circuit is limited to direct results, and the new tests cover the paginated, complete and unknown cases plus the database/sql path.

[peco-review-bot]

✅ No issues identified by the review bot.