deps: bump jaq from 2 to 3; jaq-json from 1 to 2#3653
Merged
jqnatividad merged 9 commits intomasterfrom Mar 29, 2026
Merged
Conversation
Bump jaq-core/jaq-json/jaq-std versions and update code to match the redesigned jaq API (data::JustLut, Vars, unwrap_valr, new defs/funs chaining). Adjusted compile_jaq_filter signature and loader/funs setup, replaced RcIter-based execution with the new id.run(...) pattern, converted between serde_json::Value and jaq_json::Val for inputs/outputs, and added robust handling for empty results. Updated formatting helper to handle jaq_json::Num variants. Cargo.toml and Cargo.lock updated to reflect dependency changes.
- Log warnings instead of silently dropping jaq runtime errors in both json.rs and fetch.rs (previously swallowed by filter_map(Result::ok)) - Add empty-result check in json.rs consistent with fetch.rs behavior - Log warnings when Val→serde_json::Value round-trip fails in json.rs instead of silently dropping values - Add explicit Val::TStr arm in format_val to produce unquoted strings for CSV cells, fixing spurious JSON quotes in fetch output - Update test expectations to match corrected unquoted string output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sion
The format!("{v}")→serde_json::from_str round-trip in json.rs could produce
non-JSON output for NaN, Infinity, byte strings, and non-string object keys.
Replace with a direct recursive val_to_json_value() that handles all Val
variants correctly (NaN/Inf→null, BigInt→number or string, BStr→lossy string).
Also: map Val::Null to empty string in fetch.rs format_val() so non-matching
jaq selectors produce empty CSV cells instead of literal "null", and add a
warning log when TStr contains invalid UTF-8 bytes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates qsv’s jaq/jq integration to be compatible with jaq-core v3 / jaq-json v2, and adjusts fetch tests to match the new output formatting.
Changes:
- Bump
jaq-core/jaq-stdto v3 andjaq-jsonto v2 (plus lockfile refresh). - Update
fetchandjsoncommands to use the jaq v3 execution model (Ctx+Vars+JustLut,unwrap_valr) and new defs/funs chaining. - Update
tests/test_fetch.rsexpectations for jaq-derived string/number cell values (no embedded quote characters).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Bumps jaq dependencies to latest major versions and adjusts jaq-json features. |
Cargo.lock |
Lockfile updates reflecting jaq upgrade and transitive dependency changes. |
src/cmd/json.rs |
Migrates jaq execution and adds direct Val → serde_json::Value conversion helper. |
src/cmd/fetch.rs |
Migrates jaq execution and changes scalar formatting of jaq output. |
tests/test_fetch.rs |
Updates expected outputs for jaq-derived values in fetch-related tests. |
Remove the #[ignore] attributes from all fetch and fetchpost tests that were temporarily disabled due to zippotam.us and httpbin.org API availability issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…aving as dead comments The previous commit commented out the #[ignore] attributes rather than removing them. Delete the 34 dead comment lines entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…age truncation Use char_indices().nth(50) instead of byte-offset slicing to safely truncate garbled gzip response strings in assertion messages, avoiding panic if the response contains multi-byte UTF-8 sequences. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
tests/test_fetch.rs:13
- These tests now run without
#[ignore]and make live requests tohttps://api.zippopotam.us/.... That introduces an external-network dependency and brittle assertions on the exact JSON string returned by a third-party service, which can make CI flaky/non-deterministic. Consider re-adding#[ignore](or gating with an env var) and/or switching to a local mock server/recorded fixtures so the responses are deterministic.
#[test]
fn fetch_simple() {
let wrk = Workdir::new("fetch");
wrk.create(
"data.csv",
vec![
svec!["URL"],
svec!["https://api.zippopotam.us/us/99999"],
svec![" https://api.zippopotam.us/us/90210 "],
tests/test_fetch.rs:572
- This test hits
http://httpbin.org/getdirectly. Running integration tests against public services can fail due to networking/DNS/TLS issues or service availability changes, making CI flaky. Prefer using a local mock HTTP server (like the Actix test server used later in this file) or keep the test ignored/gated behind an env var.
#[test]
fn fetch_custom_header() {
let wrk = Workdir::new("fetch");
wrk.create(
"data.csv",
vec![svec!["URL"], svec!["http://httpbin.org/get"]], // DevSkim: ignore DS137138
);
tests/test_fetch.rs:1268
fetchpost_compress_testdepends onhttps://httpbin.org/postbeing reachable and behaving consistently. Since this is an external service (and the test explicitly notes its behavior can vary), this can still be flaky in CI even with the loosened assertions. Consider moving this to a local mock server endpoint under our control (or gating/ignoring it) so the test suite remains deterministic.
#[test]
fn fetchpost_compress_test() {
let wrk = Workdir::new("fetch");
wrk.create(
"data.csv",
vec![
svec!["URL", "col1", "number col", "bool_col"],
svec!["https://httpbin.org/post", "a", "42", "true"],
svec!["https://httpbin.org/post", "b", "3.14", "false"],
svec!["https://httpbin.org/post", "c", "666", "true"],
svec!["https://httpbin.org/post", "d", "33", "true"],
svec!["https://httpbin.org/post", "e", "0", "false"],
],
src/cmd/pivotp.rs:244
build_total_rowindexesindex_cols[0]/index_cols[1]and the new#[allow(clippy::missing_asserts_for_indexing)]suppresses the lint instead of documenting/enforcing the preconditions. Consider adding an explicit guard (returning aCliErrorifindex_colsis empty, and/or adebug_assert!for the minimum length) and then removing the allow, so future call sites can’t accidentally introduce a panic.
/// Build a single-row DataFrame containing a total row.
/// Numeric columns get their sum; index columns get the provided labels.
#[allow(clippy::missing_asserts_for_indexing)]
fn build_total_row(
df: &DataFrame,
index_cols: &[String],
first_index_label: &str,
second_index_label: &str,
) -> CliResult<DataFrame> {
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.