Skip to content

Kill the naked int: ZstdCompressionLevel value type#97

Merged
dfa1 merged 1 commit into
mainfrom
feature/zstd-compression-level-type
Jul 21, 2026
Merged

Kill the naked int: ZstdCompressionLevel value type#97
dfa1 merged 1 commit into
mainfrom
feature/zstd-compression-level-type

Conversation

@dfa1

@dfa1 dfa1 commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds ZstdCompressionLevel, a record wrapping the compression level as a validated int — the compact constructor checks it against Zstd.minCompressionLevel()/maxCompressionLevel() and throws IllegalArgumentException out of range, instead of deferring to native clamping/errors. Ships DEFAULT/FASTEST/MAX constants and an of(int) factory.
  • Replaces every naked int level parameter across the public API with it: Zstd.compress, Zstd.estimateCompressContextSize/estimateCompressDictSize, ZstdCompressContext.level, ZstdCompressStream, ZstdOutputStream, ZstdCompressDictionary, ZstdDictionary.compressDict/trainCover/trainFastCover/finalizeFrom.
  • Zstd.min/max/defaultCompressionLevel() deliberately keep returning raw int — they're the bound queries ZstdCompressionLevel's own constructor validates against.
  • Breaking change (pre-1.0, no compatibility shims). CHANGELOG updated under [Unreleased].

Closes #93.

Filed #96 as a separate follow-up for a possibly analogous "size" value type (maxDictBytes/dictSize/pledgedSrcSize/maxSize) — kept out of this PR's scope on purpose.

Test plan

  • ./mvnw -pl zstd,integration-tests,benchmark -am clean verify — build success, 284 unit + 85 integration tests, 0 failures
  • ./mvnw -pl zstd validate — checkstyle clean
  • ./mvnw -pl zstd javadoc:javadoc — zero output (failOnError/failOnWarnings)
  • Added ZstdCompressionLevelTest covering valid construction, of(), the DEFAULT/FASTEST/MAX constants against the library bounds, and IllegalArgumentException at one-below-min/one-above-max
  • Reviewed with an independent pass focused on .value() extraction correctness at every native-call boundary, the ZDICT 0-default sentinel, and stale test assertions — no issues found

🤖 Generated with Claude Code


/// Validates `value` against the linked libzstd's accepted range.
public ZstdCompressionLevel {
int min = Zstd.minCompressionLevel();

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so let's hide the public constructor

@dfa1

dfa1 commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Re: hiding the public constructor — Java won't let us. A record's canonical constructor can't be more restrictive than the record type itself, and ZstdCompressionLevel has to stay a public record (it's used as a parameter/return type all over the public API). Confirmed with javac:

error: invalid canonical constructor in record ZstdCompressionLevel
  private ZstdCompressionLevel {
  (attempting to assign stronger access privileges; was public)

So new ZstdCompressionLevel(int) stays technically public. Added a doc note in 617a1c2 pointing at of(int)/DEFAULT/FASTEST/MAX as the intended entry points instead of pretending the constructor is hidden.

The alternative would be dropping the record for a hand-written final class with a private constructor + manual equals/hashCode/toString, which would truly hide it — happy to switch to that instead if you'd rather have real enforcement over the doc note.

@dfa1

dfa1 commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Dropped of(int) in a3c0b3a — call new ZstdCompressionLevel(n) directly everywhere now, per your comment. Full build/tests/checkstyle/javadoc all still clean.

/// Validates `value` against the linked libzstd's accepted range.
public ZstdCompressionLevel {
int min = Zstd.minCompressionLevel();
int max = Zstd.maxCompressionLevel();

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, would it be possible to cache these 2 values? otherwise every new level will leads to 2 native calls... is that necessary for some reason?

}

private static ZstdDictionary optimize(List<byte[]> samples, int maxDictBytes,
int compressionLevel, boolean fast) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need fast as boolean? I guess we could derive it from the CompressionLevel no?

@dfa1

dfa1 commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Re: caching min/max — good catch, no reason not to. Cached both in bcd841a as static fields (MIN_ACCEPTED/MAX_ACCEPTED), queried once at class-init instead of on every construction. ZSTD_minCLevel()/ZSTD_maxCLevel() are fixed for the life of the process (pure functions of the linked library, no runtime state), so this is safe.

@dfa1

dfa1 commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Re: the fast boolean — this one's orthogonal to ZstdCompressionLevel, so I didn't fold it into this PR, but to answer: fast picks the training algorithm (ZDICT_optimizeTrainCover vs ZDICT_optimizeTrainCover_fastCover — COVER vs fast-COVER, a build-time quality/speed tradeoff for how the dictionary itself gets constructed), while compressionLevel is the level the finished dictionary will later be used at for compression. They're independent knobs — e.g. you can fast-COVER-train a dictionary tuned for level 19, or slow-COVER-train one tuned for level 1. fast isn't derivable from the level; it's a separate axis (trainCover vs trainFastCover in the public API already keys off it via two distinct method names rather than a boolean parameter, for what it's worth — optimize(...)'s internal boolean is just the private plumbing between them).

Introduces ZstdCompressionLevel, a record wrapping the compression level as a
validated int, checked in its compact constructor against
Zstd.minCompressionLevel()/maxCompressionLevel() (IllegalArgumentException if
out of range) rather than deferring to native clamping/errors. The accepted
range is queried from libzstd once and cached, since it never changes for the
life of the process. Ships DEFAULT/FASTEST/MAX constants for the common
cases; construct directly (new ZstdCompressionLevel(n)) for any other level —
there is no separate of(int) factory, since a public record's canonical
constructor can't be hidden anyway.

Replaces every naked int level parameter with it across Zstd,
ZstdCompressContext, ZstdCompressStream, ZstdCompressDictionary,
ZstdDictionary, and ZstdOutputStream. Zstd.min/max/defaultCompressionLevel()
keep returning int, since they're the bound queries the new type validates
against.

Breaking change; the library is pre-1.0 so no compatibility shims are added.

Closes #93.
@dfa1
dfa1 force-pushed the feature/zstd-compression-level-type branch from bcd841a to 9a72557 Compare July 21, 2026 21:19
@dfa1
dfa1 merged commit 4a86470 into main Jul 21, 2026
1 check passed
@dfa1
dfa1 deleted the feature/zstd-compression-level-type branch July 21, 2026 21:24
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.

Consider a ZstdLevel value type instead of raw int level

1 participant