Conversation
There was a problem hiding this comment.
Pull request overview
Updates the project’s embedded jq/jaq execution to use jaq v3 APIs, aligning the JSON query engine with the newer jaq-core/jaq-std/jaq-json crate versions.
Changes:
- Bumped jaq dependencies to v3 (jaq-core/jaq-std) and jaq-json v2.
- Updated
run_jaqimplementation to the jaq v3 loader/compiler/context APIs. - Adjusted JSON value interchange by parsing input/output through jaq-json read/printing paths.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/json.rs |
Migrates filter loading/compilation/execution to jaq v3 APIs and updates how inputs/outputs are converted. |
Cargo.toml |
Upgrades jaq-related crate versions to v3-compatible releases. |
Cargo.lock |
Lockfile refresh reflecting the jaq dependency upgrades and their transitive changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let json = serde_json::from_str(&text).map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "failed to convert jaq output to JSON value (possibly non-JSON output): {e}" | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
val.to_string() followed by serde_json::from_str is both expensive (format + re-parse for every output item) and can introduce behavior gaps (e.g., jaq v3’s JSON Val can represent big integers via num-bigint, which serde_json may fail to parse if they exceed its numeric limits). Consider using a jaq-json writer API (if available) to emit JSON bytes and parse with serde_json::from_slice to avoid intermediate String allocations, and/or explicitly document/handle outputs that can’t be represented as serde_json::Value (e.g., fall back to returning strings for non-representable numbers).
| let json = serde_json::from_str(&text).map_err(|e| { | |
| anyhow::anyhow!( | |
| "failed to convert jaq output to JSON value (possibly non-JSON output): {e}" | |
| ) | |
| })?; | |
| let json = match serde_json::from_str(&text) { | |
| Ok(v) => v, | |
| Err(_) => Value::String(text), | |
| }; |
| let input = serde_json::to_vec(input) | ||
| .map_err(|e| anyhow::anyhow!("failed to serialize input JSON: {e}"))?; | ||
| let input = read::parse_single(&input) | ||
| .map_err(|e| anyhow::anyhow!("failed to parse input into jaq value: {e}"))?; |
There was a problem hiding this comment.
This converts each serde_json::Value into bytes and then reparses it into jaq_json::Val (to_vec -> read::parse_single). That’s an encode/decode round-trip per item and is likely significantly slower than the previous Val::from(input.clone()) path. If jaq-json v2 provides a direct conversion from serde_json::Value (or can via an optional feature), prefer that; otherwise consider keeping the original JSON as bytes during initial deserialization so run_jaq can pass slices directly to read::parse_single without re-serializing every element.
SSIA