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
Copy link
Copy Markdown
Owner

@apstndb apstndb commented May 30, 2026

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
Copy link
Copy Markdown

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

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
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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
Copy link
Copy Markdown

Copilot AI left a comment

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
Copy link
Copy Markdown
Owner Author

apstndb commented May 30, 2026

/gemini review

Copy link
Copy Markdown

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

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
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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>
Copy link
Copy Markdown

Copilot AI left a comment

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