Skip to content

Add lazy jq input with wader/gojq (jqresult package)#37

Merged
apstndb merged 15 commits into
mainfrom
feature/jqresult-lazy-gojq
May 30, 2026
Merged

Add lazy jq input with wader/gojq (jqresult package)#37
apstndb merged 15 commits into
mainfrom
feature/jqresult-lazy-gojq

Conversation

@apstndb

@apstndb apstndb commented May 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • Introduce jqresult package with eager (full ResultSet) and lazy (JQValue root with streaming rows) jq input modes via wader/gojq.
  • Add --jq-input-mode=eager|lazy (default eager); lazy mode is incompatible with experimental_csv.
  • Expand top-level gojq.Iter output to JSONL-style documents; normalize nested Iter values for JSON/YAML encode.
  • Address review findings: metadata primed after first Next, .rows available after stats drain, Stop() no longer scans all rows.

Test plan

  • go test ./...
  • Run a lazy query: --jq-input-mode=lazy --filter '.rows[]'
  • Run stats-only filter: --jq-input-mode=lazy --filter '.stats.queryPlan'
  • Verify {stats:.stats, rows:.rows[]} after stats access still emits rows

Made with Cursor

Stream rows through JQValue in lazy mode while eager mode keeps the full ResultSet. Fix metadata priming, stats-then-rows materialization, and cleanup that no longer drains unconsumed rows.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copilot AI review requested due to automatic review settings May 30, 2026 17:01

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request replaces itchyny/gojq with wader/gojq and introduces a new jqresult package to support both eager and lazy evaluation of Spanner query results. Feedback on the changes highlights critical concurrency and performance issues: jqresult/lazy.go has potential race conditions and lock contention during network I/O, which can be resolved by introducing a separate I/O mutex with double-checked locking. Additionally, RowToJSON in jqresult/rowjson.go is highly inefficient due to unnecessary JSON marshaling and unmarshaling, and should be optimized by directly utilizing AsSlice().

Comment thread jqresult/lazy.go
Comment thread jqresult/lazy.go
Comment thread jqresult/rowjson.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a jqresult package to support eager and lazy jq input modes using wader/gojq, wiring the new mode through CLI flag parsing and output execution.

Changes:

  • Adds --jq-input-mode=eager|lazy with lazy JQValue support for streaming rows and deferred stats access.
  • Moves jq execution, normalization, printing, protojson conversion, and row iteration helpers into jqresult.
  • Updates documentation, Go version, CI Go setup, and jq-related dependencies.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
README.md Documents Go 1.24 and new jq input modes.
main.go Parses jq mode and routes eager/lazy jq output execution.
jqresult/compile.go Adds jq parse/compile helper.
jqresult/jqresult_test.go Adds unit tests for normalization, lazy behavior, and printing.
jqresult/lazy.go Implements lazy jq root value for metadata, rows, and stats.
jqresult/mode.go Defines jq input modes and format validation.
jqresult/normalize.go Normalizes iterators and JQValue outputs before encoding.
jqresult/pipeline.go Executes jq in eager or lazy mode.
jqresult/print.go Prints jq iterator output with top-level iterator expansion.
jqresult/protojson.go Converts protobuf ResultSets and stats to JSON-compatible maps.
jqresult/rowiter.go Streams Spanner rows as jq-compatible values.
jqresult/rowjson.go Converts Spanner rows to protojson-shaped row arrays.
go.mod Updates Go version and replaces jq dependency.
go.sum Updates dependency checksums.
.github/workflows/go.yml Updates CI Go version.
.github/workflows/golangci-lint.yml Updates lint workflow Go version.
.github/workflows/ko.yml Updates ko workflow Go version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread jqresult/rowiter.go
Serialize lazy Spanner I/O with ioMu, drain iterators fully when redacting rows, use ListValue.AsSlice for row encoding, and bump golangci-lint to v1.64.8 for Go 1.24.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Comment thread jqresult/pipeline.go
Comment thread main.go
Reject nil RowIterator in jqresult.Execute lazy mode and cover lazy filters with a real Spanner RowIterator in integration tests.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comment thread integration_test.go
Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comment thread jqresult/lazy.go
Expose stats via lazyStatsField so to_entries and similar filters observe the same values as direct .stats access.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Comment thread jqresult/lazy.go Outdated
Comment thread main.go Outdated
lazyStatsField reports the real stats map length after drain, and partitioned DML returns a clear error instead of silently using eager input.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comment thread jqresult/lazy.go Outdated
Object literals that store .rows before .stats now replay materialized rows after drain instead of a stopped RowIter.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comment thread main.go Outdated
Partitioned DML jq output is already handled by the eager runJqOutput path via runInNewTransaction.

Co-authored-by: Cursor <cursoragent@cursor.com>
Preserve rows already read via .rows[] before .stats drains the iterator.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Comment thread jqresult/normalize.go Outdated
Comment thread jqresult/lazy.go
Use non-nil empty slices in normalizeIter and after lazy drain so zero-row results match documented array shape.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comment thread jqresult/lazy.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.

Comment thread jqresult/lazy.go Outdated
Co-authored-by: Cursor <cursoragent@cursor.com>
@apstndb

apstndb commented May 30, 2026

Copy link
Copy Markdown
Owner Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new jqresult package to support lazy processing of Spanner query results using an embedded wader/gojq library, adding a --jq-input-mode flag with eager and lazy options. It also updates the project's Go version to 1.24.0 and updates dependencies. The review feedback highlights a critical correctness bug in lazyRowsField related to row replay and partial consumption when using multiple iterators or late stream completion. Additionally, minor code simplifications are suggested in jqresult/normalize.go to remove a redundant check and eliminate the resulting dead code.

Comment thread jqresult/lazy.go
Comment thread jqresult/normalize.go
Comment thread jqresult/normalize.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Comment thread jqresult/lazy.go Outdated
Comment thread jqresult/lazy.go
Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Comment thread jqresult/lazy.go
Comment thread jqresult/lazy.go
Read rowsStreamDone under mu, return lazyRowsField for redacted rows, and serve array ops from cached rows after streaming completes.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

Comment thread jqresult/rowjson.go
Comment thread jqresult/lazy.go
Restore per-row protojson decoding with UseNumber for numeric parity, and avoid materializing all rows in lazyRowsField.JQValueEach while streaming.

Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.

@apstndb apstndb merged commit 6de4850 into main May 30, 2026
3 checks passed
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