Skip to content

refactor(decoder): delegate JSONFormatConfig to spanvalue v0.1.9#582

Merged
apstndb merged 2 commits intomainfrom
refactor/spanvalue-jsonformatconfig
Mar 27, 2026
Merged

refactor(decoder): delegate JSONFormatConfig to spanvalue v0.1.9#582
apstndb merged 2 commits intomainfrom
refactor/spanvalue-jsonformatconfig

Conversation

@apstndb
Copy link
Copy Markdown
Owner

@apstndb apstndb commented Mar 27, 2026

Summary

Replace the local JSON formatting implementation in decoder/jsonvalue.go (106 lines of formatting logic) with a one-line delegation to spanvalue.JSONFormatConfig() from spanvalue v0.1.9. The comprehensive type coverage tests (302 lines) are now maintained upstream in spanvalue; only a smoke test remains here.

This is a follow-up to #581 which added type-aware JSONL output. The JSON formatting logic was subsequently upstreamed to spanvalue#1 with improvements (ENUM as number, nil Value safety, structpb.MarshalJSON delegation, etc.).

Key Changes

  • decoder/jsonvalue.go: 106 lines → 8 lines (one-line delegation to spanvalue.JSONFormatConfig())
  • decoder/jsonvalue_test.go: 302 lines → 40 lines (smoke test only; full coverage in spanvalue)
  • go.mod: spanvalue v0.1.8 → v0.1.9, spantype v0.3.8 → v0.3.9

Development Insights

Discoveries

  • Upstreaming format logic to spanvalue enabled improvements that would have been harder in spanner-mycli alone: ENUM handling as number, nil Value safety fix in isNull, structpb.Value.MarshalJSON() delegation for most types
  • spanvalue v0.1.9 also includes FormatRowJSONObject, FormatCompactArray, FormatJSONObjectStruct, UnnamedFieldNamer and other building blocks that could simplify future JSONL enhancements

Test Plan

  • make check passes
  • Smoke test verifies delegation works (INT64 42 → JSON number 42)
  • Full type coverage maintained in spanvalue's own test suite

Replace the local JSON formatting implementation (106 lines) with a
one-line delegation to spanvalue.JSONFormatConfig(). The comprehensive
type coverage tests are now maintained in spanvalue; only a smoke test
remains here.

Also update spantype to v0.3.9 (adds typector type constant functions).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 27, 2026 20:43
Copy link
Copy Markdown
Contributor

@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 updates the spantype and spanvalue dependencies to versions 0.3.9 and 0.1.9, respectively. The JSONFormatConfig in the decoder package has been refactored to delegate directly to the spanvalue library, allowing for the removal of redundant local formatting logic and extensive test cases. I have no feedback to provide.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 27, 2026

Code Metrics Report

📊 View detailed coverage report (available for 7 days)

main (6f62e81) #582 (95b7bb8) +/-
Coverage 70.9% 70.8% -0.2%
Code to Test Ratio 1:1.3 1:1.3 -0.1
Test Execution Time 1m12s 1m15s +3s
Details
  |                     | main (6f62e81) | #582 (95b7bb8) |  +/-  |
  |---------------------|----------------|----------------|-------|
- | Coverage            |          70.9% |          70.8% | -0.2% |
  |   Files             |             78 |             78 |     0 |
  |   Lines             |           7094 |           7075 |   -19 |
- |   Covered           |           5036 |           5014 |   -22 |
- | Code to Test Ratio  |          1:1.3 |          1:1.3 |  -0.1 |
  |   Code              |          16470 |          16419 |   -51 |
- |   Test              |          21619 |          21376 |  -243 |
- | Test Execution Time |          1m12s |          1m15s |   +3s |

Code coverage of files in pull request scope (84.6% → 81.6%)

Files Coverage +/- Status
internal/mycli/decoder/jsonvalue.go 100.0% +5.0% modified
internal/mycli/execute_sql.go 80.0% -1.4% affected
internal/mycli/metrics/execution_metrics.go 86.6% -4.5% affected

Reported by octocov

Copy link
Copy Markdown
Contributor

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

Refactors the decoder’s JSON value formatting to rely on the upstream spanvalue implementation, simplifying local maintenance and aligning JSONL output behavior with the shared library.

Changes:

  • Delegate decoder.JSONFormatConfig() to spanvalue.JSONFormatConfig() (removes local formatting logic).
  • Replace comprehensive local JSON formatting tests with a minimal smoke test.
  • Bump dependencies: spanvalue to v0.1.9 and spantype to v0.3.9 (plus corresponding go.sum updates).

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
internal/mycli/decoder/jsonvalue.go Removes local JSON formatting implementation and delegates to spanvalue.JSONFormatConfig().
internal/mycli/decoder/jsonvalue_test.go Collapses detailed type coverage tests into a single delegation smoke test.
go.mod Updates spanvalue and spantype versions to pick up upstream JSON formatting behavior.
go.sum Updates module checksums to match the dependency bumps.

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

Cover NULL, STRING, BOOL, JSON pass-through, ARRAY, and STRUCT
in addition to INT64 to detect regressions from spanvalue upgrades.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@apstndb
Copy link
Copy Markdown
Owner Author

apstndb commented Mar 27, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@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 updates the spantype and spanvalue dependencies to version 0.3.9 and 0.1.9, respectively. The JSONFormatConfig implementation in the decoder package has been refactored to delegate directly to the spanvalue library, allowing for the removal of redundant formatting logic. Additionally, the associated unit tests were simplified by utilizing the gcvctor helper package. I have no feedback to provide.

Copy link
Copy Markdown
Contributor

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 3 out of 4 changed files in this pull request and generated no new comments.


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

@apstndb
Copy link
Copy Markdown
Owner Author

apstndb commented Mar 27, 2026

@gemini-code-assist /request-summary

@apstndb apstndb merged commit bfe804c into main Mar 27, 2026
6 checks passed
@apstndb apstndb deleted the refactor/spanvalue-jsonformatconfig branch March 27, 2026 21:10
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

This pull request simplifies the codebase by offloading JSON formatting responsibilities to the spanvalue library. By centralizing this logic, the project benefits from improved type handling and consistency, while significantly reducing maintenance overhead in the local decoder implementation.

Highlights

  • Code Consolidation: Replaced 106 lines of custom JSON formatting logic in decoder/jsonvalue.go with a direct delegation to spanvalue.JSONFormatConfig().
  • Dependency Update: Updated spanvalue to v0.1.9 and spantype to v0.3.9 in go.mod to support the upstreamed formatting logic.
  • Test Refactoring: Reduced decoder/jsonvalue_test.go from 302 lines to 40 lines, retaining only essential smoke tests while relying on upstream coverage.
Activity
  • Pull request created by apstndb.
  • Code metrics report generated by github-actions[bot].
  • Copilot provided feedback on test coverage, suggesting additional smoke tests for the JSONL pipeline contract.
  • Author addressed feedback by adding representative test cases for NULL, STRING, BOOL, JSON pass-through, ARRAY, and STRUCT types.
  • Author requested a summary of the pull request.

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