feat(qsv)!: bump MINIMUM_QSV_VERSION from 4.0.0 to 20.0.0#275
Merged
Conversation
DP+ now requires qsv >= 20.0.0. The version check in qsv_utils.QSVCommand._check_version() refuses to start otherwise. Why now: - 16 qsv major releases since 4.0.0 brought useful perf & correctness fixes (frequency parallel chunk sizing, stats n_counts columns, BLAKE3 fingerprints, safenames byte-cap, geocode/excel/safenames hardening). - The PR #255 bug class (frequency table COPY column mismatch) is fully eliminated by the v2.0 stage refactor — but other older-qsv quirks still bite, so a coordinated floor bump is the cleanest path forward. Operator-facing breakage (full notes in CHANGELOG.md): - safe_header_names is now byte-capped (≤60 bytes incl. suffix). Long- ASCII duplicates and multibyte headers ingested under qsv ≤19.1.0 may get *different* Postgres column names on resubmit under qsv 20. - If `summary_stats_options = "--percentiles"` is set, the consolidated `percentiles` stats column now emits `<N>: <val>` entries instead of bare values (qsv 12.0.0). DP+ does not pass --percentiles by default. What's in the change: - ckanext/datapusher_plus/config.py: MINIMUM_QSV_VERSION → 20.0.0 - README.md: install-snippet example version bumped to 20.0.0 - CHANGELOG.md: Unreleased entry with migration notes - docs/qsv-20.0.0-upgrade-test-plan.md: regression test plan with fixture matrix, smoke-test matrix, and re-ingestion checks - tests/test_qsv_v20_regression.py: 14 pytest cases asserting qsv 20.0.0 contracts (safe_header_names byte-cap, safenames verify-mode JSON shape, stats percentile-label-prefix format). Skips cleanly if qsv is missing or older than 20.0.0. - tests/static/safenames_*.csv, numeric.csv: 6 hand-crafted fixtures covering ASCII-short (control), ASCII-duplicate-long-collision, CJK, emoji-with-trim, exact-duplicate-suffix, and a numeric-stats fixture. - .github/workflows/ci.yml: QSV_VER default 9.1.0 → 20.0.0; new pre- flight step "Run qsv contract regression tests" gates the heavy CKAN integration steps with a <1-minute pytest run. - .github/workflows/main.yml: QSV_VER 7.1.0 → 20.0.0. - .gitignore: ignore qsv stats/freq cache files under tests/static. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- tests: switch test_long_multibyte_header_fits_60_bytes to --mode a so
the safe_header_names byte-cap codepath is exercised unconditionally
(under --mode c the result was qsv-classification-dependent: CJK is
preserved as quoted identifier, accented Latin is rewritten — fragile).
- tests/static/safenames_emoji.csv: rebuild fixture so the trim cases
(' col 1 ', '"quoted col"') stay in unsafe_headers under qsv 20 (they
retain embedded spaces, so they're still unsafe after surrounding-trim).
Previously the headers landed in safe_headers, making the trim
assertion vacuous. Added explicit membership checks for the post-trim
forms so a regression in the trim contract surfaces clearly.
- tests: add `assert QSV_BIN is not None` to _run() so any future test
added without @requires_qsv_20 fails with a clear message instead of
TypeError on list concatenation.
- ci.yml: switch pre-flight step from `pytest` to `python3 -m pytest`
and add `pip install --quiet pytest` so the step is self-contained
regardless of where pip installed pytest's shim. pytest is already in
requirements-dev.txt:2 (installed earlier in the job), but the
belt-and-suspenders install is cheap insurance.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ckan/ckan-dev:2.11 container's site-packages registers a pytest plugin (ckan.tests.pytest_ckan.ckan_setup) that auto-loads on every pytest invocation and calls make_app(conf) in pytest_sessionstart. That demands a fully-configured CKAN (SECRET_KEY, datastore URLs, etc.) which doesn't exist until the later "Setup CKAN configuration" step. Result on PR #275 first CI run: the pre-flight failed with `ckan.exceptions.CkanConfigurationException: SECRET_KEY: Missing value` before any of our regression tests could execute. Fix: set PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 on the step so the CKAN plugin can't hijack the session. Our tests only need core pytest (subprocess + skipif markers) — no CKAN, no scheming, no other plugins. Also drops the redundant `pip install --quiet pytest` line: pytest is already installed via requirements-dev.txt at the earlier "Install Python dependencies" step, and the install was never the issue (the auto-loaded CKAN plugin was). Verified locally that the same invocation under PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 produces 14/14 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qsv ≤ 9.1.0 reported `record_count: 1` for header-only CSVs (the header row got counted), while qsv ≥ 10.0.0 correctly reports `record_count: 0`. ValidationStage was propagating that value into `dataset_stats.RECORD_COUNT`, which AnalysisStage then consumed to decide whether to short-circuit on the zero-records branch (analysis.py:62-68). The cross-version mismatch surfaced on PR #275's CI: `sample-excel-file.xlsx` is a header-only XLSX. Under qsv 9.1.0 (CI on `main`) sortcheck's buggy count of 1 propagated through and DP+ created an empty datastore table (`datastore_active=true`). Under qsv 20.0.0 (this PR), the corrected count of 0 made AnalysisStage abort with "Upload skipped as there are zero records" → `datastore_active=false`. `qsv count` is data-rows-only across all qsv versions DP+ supports, so let AnalysisStage compute RECORD_COUNT via `_count_records` (which it already does as the fallback path at analysis.py:62-64). Drop the sortcheck-side seeding entirely; keep IS_SORTED/UNSORTED_BREAKS/DUPE_COUNT stats (those have always been correct). This is a behavior normalization, not a behavior preservation: header- only files will now consistently short-circuit on all qsv versions, not just qsv 20+. That's the intended path — the "empty datastore table for header-only files" outcome on qsv 9.1.0 was an accident of the sortcheck bug. Locally verified the qsv 20 regression suite is unaffected (14/14 pass). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Heads-up for reviewers — three follow-up commits + an external dependency since the first push: Commits since initial review
External dependencyCI on this branch will stay red until dathere/datapusher-plus_testing#12 merges. That PR replaces a header-only XLSX fixture ( Merge order
Full diagnosis of why the fixture issue surfaced (and why patching DP+ to preserve the old "create empty datastore tables" behavior would have been the wrong fix) is in the PR description under "Follow-up dependency: testing-repo fixture". |
jqnatividad
added a commit
to dathere/datapusher-plus_testing
that referenced
this pull request
May 12, 2026
…#12) The previous fixture was an XLSX containing only a header cell ('Sample Excel File' at A1) with no data rows — XLSX dimension `ref="A1"`. Despite the name, it tested nothing meaningful about XLSX ingestion: there was no data for DP+ to type-infer, COPY, or index. It was "passing" on `main` only because qsv ≤ 9.1.0's `sortcheck` had an off-by-one bug that counted the header row as a record, which DP+ propagated through `dataset_stats.RECORD_COUNT` and used to skip the zero-records short-circuit at AnalysisStage. qsv ≥ 10.0.0 fixed that count, and DP+ PR #275 (the qsv 20.0.0 minimum bump) drops the sortcheck-based seeding in favor of `qsv count` — at which point the fixture's accidental success path collapses: header-only files now correctly short-circuit with `datastore_active=false`, and the CI marks this fixture FAIL. Replace with a small real XLSX: 3 data rows × 4 typed columns chosen to exercise DP+'s type inference pipeline end-to-end: id (Integer) | name (String) | hire_date (Date) | salary (Float) ------------ | ------------- | ---------------- | -------------- 1 | Alice | 2023-01-15 | 50000.0 2 | Bob | 2022-07-30 | 65000.5 3 | Carol | 2024-03-10 | 72000.75 Verified locally under qsv 20.0.0: - `qsv excel --trim` → 3 4-column rows - `qsv count` → 3 - `qsv sortcheck --json` → record_count=3, sorted=true, dupe_count=0 This unblocks dathere/datapusher-plus#275. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dathere/datapusher-plus_testing#12 has merged. The replacement sample-excel-file.xlsx now has 3 typed data rows, so the integration- test step here should turn green. No code changes — empty commit to kick CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR raises DataPusher Plus’ minimum supported qsv version to 20.0.0 and adds regression coverage/documentation for qsv contract changes that affect ingestion behavior.
Changes:
- Bumps
MINIMUM_QSV_VERSIONand CI workflow qsv versions to 20.0.0. - Adds qsv 20 regression tests and CSV fixtures for safenames and stats behavior.
- Documents operator migration risks and manual upgrade validation steps.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
ckanext/datapusher_plus/config.py |
Raises the qsv minimum version gate. |
ckanext/datapusher_plus/jobs/stages/validation.py |
Stops using sortcheck record counts as canonical row counts. |
tests/test_qsv_v20_regression.py |
Adds qsv 20 contract regression tests. |
tests/static/safenames_emoji.csv |
Adds safenames verify-mode fixture. |
tests/static/safenames_dupe_suffix.csv |
Adds duplicate-header fixture. |
tests/static/safenames_cjk.csv |
Adds multibyte-header fixture. |
tests/static/safenames_ascii_short.csv |
Adds safe ASCII-header fixture. |
tests/static/safenames_ascii_duplicate_long.csv |
Adds long duplicate-header fixture. |
tests/static/numeric.csv |
Adds stats percentile fixture. |
README.md |
Updates qsv install example to 20.0.0. |
CHANGELOG.md |
Adds breaking-change and migration notes. |
docs/qsv-20.0.0-upgrade-test-plan.md |
Adds manual upgrade/regression checklist. |
.github/workflows/ci.yml |
Installs qsv 20.0.0 and runs contract tests before CKAN setup. |
.github/workflows/main.yml |
Updates workflow qsv version to 20.0.0. |
.gitignore |
Ignores qsv-generated static fixture cache files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot's review on PR #275 caught that §2.3's expected result described qsv `safenames --mode a` (always-rewrite) behavior — claiming CJK headers become `unsafe_*` ASCII forms — while DP+'s production path calls `safenames --mode conditional` (analysis.py:177). Conditional mode preserves quoted-identifier-safe headers verbatim, including CJK and embedded spaces. The plan and the regression suite (`test_conditional_mode_preserves_cjk_as_quoted_identifiers`) were internally inconsistent. The same drift affected §2.2 (long ASCII duplicate) and §2.4 (emoji), so fix the whole matrix in one pass: - Add a `[!NOTE]` callout at the top of "Test matrix" explaining that DP+ uses --mode c and what that preserves. - §2.2: both long headers preserved verbatim as quoted identifiers (not suffixed) under --mode c. Note the --mode a byte-cap outcome lives in the regression suite. - §2.3: CJK preserved verbatim as quoted Unicode identifiers (e.g. `"顧客識別子"`), not romanized. - §2.4 (emoji): clarify the mixed outcome — emoji headers rewritten, whitespace/quote-wrapped headers trimmed-and-preserved. - §2.5: unchanged (col,col,col,col → col,col_2,col_3,col_4 holds under --mode c too, since duplicate-suffixing is always applied). Each row now cross-references the specific regression test in `tests/test_qsv_v20_regression.py` that pins the underlying qsv contract, keeping the manual plan and the automated suite in sync going forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Summary
MINIMUM_QSV_VERSIONfrom4.0.0→20.0.0(ckanext/datapusher_plus/config.py:34). DP+ refuses to start under older qsv via the existing check atqsv_utils.QSVCommand._check_version().tests/test_qsv_v20_regression.py, 14 cases) that asserts the qsv 20.0.0 contracts DP+ depends on, plus 6 hand-crafted CSV fixtures undertests/static/.ci.ymlas a fast pre-flight step that fails in <1 minute if qsv ever ships a version that breaks DP+'s expectations.CHANGELOG.mdand provides a manual regression test plan indocs/qsv-20.0.0-upgrade-test-plan.md.This is breaking for operators but not for end users — once the qsv binary is bumped, default-config deployments are unaffected.
Operator migration notes
Full notes in
CHANGELOG.md. The two changes most likely to bite real deployments:safe_header_namesis now byte-capped (≤60 bytes including duplicate-disambiguation suffix). Resources ingested under qsv ≤19.1.0 with long-ASCII duplicates or multibyte headers may get different Postgres column names on resubmit. ASCII-only headers without duplicates are unchanged. See test plan §5 for the re-ingestion checks.stats --percentilesconsolidated column format changed (qsv 12.0.0). DP+ does not pass--percentilesby default. Only deployments that have setckanext.datapusher_plus.summary_stats_options = \"--percentiles\"AND parse thepercentilescolumn in scheming formulas need to update those formulas to handle the new `: ` entry format.What's in the change
ckanext/datapusher_plus/config.pyMINIMUM_QSV_VERSION = \"20.0.0\"README.mdCHANGELOG.md[Unreleased]entry with full migration notesdocs/qsv-20.0.0-upgrade-test-plan.mdtests/test_qsv_v20_regression.pytests/static/safenames_*.csv(5 files) +numeric.csv.github/workflows/ci.ymlQSV_VERdefault 9.1.0 → 20.0.0; new "Run qsv contract regression tests" step before CKAN startup.github/workflows/main.ymlQSV_VER7.1.0 → 20.0.0.gitignoretests/static/(qsv 17.0.0+ auto-generates these)Test plan
tests/test_qsv_v20_regression.pypass locally against qsv 20.0.0yaml.safe_load)ci.ymlis positioned to fail-fast before CKAN/datastore/worker startupdocs/qsv-20.0.0-upgrade-test-plan.md§4 (end-to-end smoke) and §5 (re-ingestion) on a staging deploymentForward-compat note
The regression tests assert invariants (`len ≤ 60`, `count ≥ 3`, `: ` regex), not exact strings. Future qsv major bumps (21.x, etc.) should keep passing; if they don't, that's the signal that DP+ needs to react to a new contract change.
Related
🤖 Generated with Claude Code