Repository navigation
Complete NimbleCSV 1.3 parity for v0.4.0 - #5
Merged
Merged
Conversation
Rule 8 forbids a performance claim without measurement, and 0.4.0 adds strict validation to the scanner hot loop. This records the "before" so the "after" is auditable. Benches scan_structural directly, isolating it from the NIF and term-building layers, across three input shapes (plain, quoted, mixed). criterion is dev-only with a lean feature set (no plotters, no rayon in the harness) and does not enter the shipped cdylib. Baseline on aarch64-apple-darwin, saved as `pre-strict`: plain 366.34 us 2.4575 GiB/s quoted 579.10 us 2.1336 GiB/s mixed 404.13 us 2.2421 GiB/s Elixir-level baseline vs nimble_csv 1.3.0 (parse_string, 20k rows): plain 5.21x faster quoted 30.81x faster mixed 10.85x faster Reproduce with: cargo bench --bench scanner -- --baseline pre-strict Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RustyCSV parses malformed CSV leniently where nimble_csv raises. A
differential run over 22 decoding cases found exactly three divergences,
all of them malformed input; well-formed CSV already matched byte for
byte. This records those three, without acting on them yet.
The rules, lifted from nimble_csv 1.3.0 rather than from its docs:
1. A quote may only open a field at start of input or immediately
after a separator, a line feed, or another quote (separator_case/0)
2. A closing quote must be followed by a separator, a row terminator,
another quote, or end of input (newlines_escape!/1)
3. Input must not end inside a quoted field (finalize_parser/1)
All three ride along in the existing chunk loop as bitmask lookbehind.
Opening and closing quotes fall straight out of the parity mask the
scanner already computes: `quoted` is an inclusive prefix XOR, so an
opening quote has odd parity at its own position and a closing quote
even. Rule 3 was already computed and thrown away -- scan_structural
tracked quote_carry through the whole input and dropped it when building
StructuralIndex.
Expressing rule 2 as lookbehind rather than lookahead keeps the loop
from ever needing the chunk it has not read yet. The single exception is
a carriage return, which may only follow a closing quote as the first
byte of a CRLF pair, so one byte past the chunk is peeked for that.
The scanner records violations and keeps going; it never panics and it
does not change what rows come out. Raising is the Elixir layer's job,
landing next.
Measured, Rule 8. Criterion vs the `pre-strict` baseline:
plain 2.4575 -> 2.6335 GiB/s +10.2% (p = 0.00)
quoted 2.1336 -> 2.8642 GiB/s +29.4% (p = 0.00)
mixed 2.2421 -> 2.4425 GiB/s +7.2% (p = 0.00)
Validation made it faster, which is not free lunch but two incidental
wins: the extraction loops now run after the mask math instead of
interleaved with it, and the LF/CR masks are narrowed against a constant
instead of against not_quoted, which lifts them off the prefix-XOR
dependency chain. That second one is why quoted input gained most.
Correction to the previous commit: its Elixir-level ratios were taken on
a loaded machine (benchee deviation +/-63%) and should not be used. On a
quiet machine with 10s samples both libraries roughly doubled, and
rusty_csv measures 3.77x / 29.92x / 4.35x faster than nimble_csv on
plain / quoted / mixed. The criterion figures above are the sound ones.
Gates: cargo fmt clean, cargo clippy -D warnings clean, 126 Rust tests,
476 Elixir tests, and the 22-case differential still shows the same
three divergences and no others.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cargo.toml carried no [lints] table, so the three lints Rule 24 names
were never enforced. Adds them and fixes what they surfaced.
[lints.rust] unsafe_op_in_unsafe_fn = "deny"
[lints.clippy] undocumented_unsafe_blocks = "deny"
missing_safety_doc = "deny"
The only unsafe in the crate is TrackingAllocator, behind the
memory_tracking feature. It had neither a safety rationale on the
`unsafe impl` nor explicit unsafe blocks inside its two `unsafe fn`
bodies. Both are now written out: the impl states why wrapping mimalloc
or the system allocator in relaxed atomic counters preserves the
GlobalAlloc contract (the counters are statistics, not allocator state,
so a torn read skews a profiling number and cannot affect memory
safety), and each forwarding call carries its own SAFETY comment naming
the obligation the caller already owes.
Whether a trait-impl method needs its own `# Safety` section is
arguable, since GlobalAlloc defines the contract. The two unambiguous
violations were the missing rationale and the implicit unsafe ops, and
both are fixed.
Lint enforcement verified rather than assumed: a deliberate
`pub unsafe fn` dereferencing a raw pointer produces two hard errors
(E0133 for the missing block, missing_safety_doc for the absent
section), and the tree returns to clippy-clean once reverted.
Also corrects Rule 23 derive order on the two types added earlier on
this branch, both of which put Copy in the wrong position:
Violation Clone, Copy, Debug, Eq, PartialEq -> Copy, Clone, ...
QuoteCarry Clone, Copy -> Copy, Clone
Gates: cargo fmt clean, cargo clippy -D warnings clean (exit 0), 126
Rust tests pass. No behavior change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mechanical only. No behavior change, no API change, no signature change.
Kept separate from the 0.4.0 parity work so that churn does not obscure
the Phase 2 diff.
Rule 10: no source module used `//!`. Every module-level comment was an
ordinary `//` block, so none of it reached rustdoc. All 21 Rust files now
open with `//!`.
Two files needed judgement rather than substitution:
lib.rs `#![cfg_attr(not(test), deny(...))]` sat in the middle of
the comment block. The attribute is hoisted to join
`#![feature(portable_simd)]` and the whole block becomes
module doc. Inner attributes still precede all items, so
this is a move, not a semantic change.
newlines.rs opened with `///`, which attached to `pub struct Newlines`
rather than the module. Converting it wholesale would have
silently moved the struct's documentation onto the module
and left the struct undocumented. The module purpose is
now `//!` and the `is_default` behaviour stays on the
struct, where it describes an actual field.
Rule 23: six derive lists put `Copy` after `Debug`/`Clone`, or ordered the
standard traits non-lexicographically.
core/newlines.rs:7 Debug, Clone -> Clone, Debug
core/simd_index.rs:11 Debug, Clone, Copy, PartialEq, Eq
strategy/encoding.rs:7 Debug, Clone, Copy, PartialEq, Eq
strategy/general.rs:190 Debug, Clone, Copy
strategy/two_phase.rs:15 Debug, Clone, Copy
strategy/streaming.rs:32 Debug, Clone, Copy, PartialEq, Eq, thiserror::Error
All five of the remaining ones become `Copy, Clone, Debug[, Eq,
PartialEq][, third-party]`. Enumerated mechanically rather than by eye,
and the same enumerator now reports zero remaining. None of these types
derives `Ord`, `PartialOrd`, or `Hash`, so the Rule 23 field-order
exception does not apply and no derived behavior changes.
Gates: cargo fmt clean, cargo clippy -D warnings clean (exit 0), 126 Rust
tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Make strict decoding the default across batch, general, parallel, and streaming paths while preserving strict: false for dirty input. Match NimbleCSV encoding row shape and BOM behavior, reject oversized scanner input, and document the intentional redaction of CSV contents in errors. Upgrade Rustler to 0.38, refresh compatible dependencies, retain NIF 2.15-2.17 artifacts, and add Elixir 1.20 / OTP 29 to CI. Verified with 131 Rust tests, 489 ExUnit tests on Elixir 1.20.3 / OTP 29, fmt, clippy, Credo, Dialyzer, and the NimbleCSV v1.3.0 semantic suite (21/21).
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
Compatibility
This is intentionally a breaking 0.4.0 release. RustyCSV now matches NimbleCSV 1.3.0 exception categories and behavior for malformed CSV. Error messages contain byte offsets but redact raw CSV content.
NimbleCSV's v1.3.0 upstream suite passes 21/21 semantically. The literal suite is 17/21 because four assertions require NimbleCSV's raw-input error wording, which RustyCSV intentionally does not reproduce.
Issue #3 was closed as superseded: parse_string/2 remains the NimbleCSV-compatible primary API, with headers: retained as a documented RustyCSV extension.
Verification
Performance note
This PR makes no new scanner performance claim. The figures recorded in commit 2b0e89a were collected under machine load and did not all survive the later A/B/A noise-floor check. They must not be reused in release notes or documentation without a quiet-machine production-entry-point rerun.