[xiangfu0]

Standalone ingestion borrows plain local inputs to avoid staging copies, while compressed and nonlocal inputs retain owned staging. Batch generation supports opt-in Zstd archives across standalone/Spark/Hadoop; gzip remains the default for compatibility. Backfill lineage and the default uploader now recognize every supported archive codec, and all push modes honor the selected file pattern.

The job-spec usage guide is in pinot-docs #1119. The LZ4 implementations and extraction buffering have merged in #19946 and are included in this PR's master base. Duplicate-name rejection remains separate in #19947.

Archive codec comparison

Historical local benchmark at b7b1b58, before the split: the same 1M-row, 157.25 MB segment (20 files), Apple M2 Max, JDK 25, warm cache, shared host, no fsync. Compressed codecs use TarCompressionUtils: gzip defaults, Zstd level 3, and lz4-java 1.12.0 fast encoder (JNI selected) with 4 MiB independent blocks/content checksum and the existing Commons reader. Plain tar uses the same writer and an equivalent buffered Commons extractor. The buffering and fast LZ4 utility changes are now on master through #19946; this table records the historical combined snapshot. MB means 1,000,000 bytes.

Codec	Creation, seconds	Extraction, seconds	Archive, MB
tar (uncompressed)	0.133 (0.128–0.138)	0.142 (0.134–0.195)	157.26
gzip	5.372 (5.216–5.543)	0.946 (0.874–0.946)	100.77
Zstandard	0.664 (0.635–0.755)	0.293 (0.289–0.320)	91.76
LZ4 (lz4-java fast, #19946)	0.185 (0.185–0.191)	0.514 (0.495–0.524)	143.50

Times are median (min–max), three rotated runs after one warm-up per codec, including stream closing and excluding verification. All 16 round trips matched every file's SHA-256; input/source hashes stayed unchanged. These archive-only timings do not measure end-to-end ingestion or isolate the local-copy benefit.

Zstd is preferred for this workload: it is 8.09× faster to create, 3.23× faster to extract, and 8.95% smaller than gzip in the combined benchmark.

Fast LZ4 makes a usable option available: historical Commons LZ4 at b9a5bae took 1124.207 s to create / 31.355 s to extract this segment (one first-invocation sample). The fast-writer snapshot measured above produced 0.185 s / 0.514 s, with a 143.50 MB archive versus 117.56 MB before (+22.06%). Zstd remains the preferred compression tradeoff. Merged #19946 exposes lz4-fast and lz4-hc through the compressor factory and makes .tar.lz4 select lz4-fast by default. The existing lz4-framed name retains the Commons encoder, and gzip remains the overall default codec. See #19946 for fresh fast/HC measurements of that provider implementation. Extraction gains include buffering and the changed compressed stream.

Compatibility/release note: Hadoop and Spark TAR/URI push jobs now honor pushFileNamePattern, which they previously ignored. Existing specs with a pattern may push fewer segments or none; review or remove that pattern when upgrading. Empty selections now emit a WARN with the output directory and pattern and skip pushing, including standalone consistent replacement. Local URI and metadata pushes preserve literal percent sequences in filenames: a file named a%3Ab.tar.gz is addressed as a%253Ab.tar.gz, including when segmentUriPrefix is set.

Usage/upgrade: set segmentTarCompressionCodec: zstd in the job YAML after every downstream consumer supports .tar.zst. Gzip remains the compatibility default, including for mixed-version deployments, and metadata stays .metadata.tar.gz. Use a clean output directory or a narrow push pattern when switching codecs. Batch generation supports gzip/Zstd; push also accepts existing .tar.lz4 archives. Duplicate-name rejection is no longer part of this PR; see #19947 for its separate upgrade note.

Validation: 53 focused reactor tests passed on JDK 25 after rebasing onto master f57c2285ec, including empty consistent-push selection and literal percent-encoded filenames across gzip/LZ4/Zstd, with and without a URI prefix. Affected-module formatting, license, and style checks and independent Pinot review passed.

Fixes/reproduction: METADATA push with an unset output.segment.dir.uri and a configured URI prefix previously hit a null dereference; the archive helper now preserves that supported configuration. Creating two LZ4 archives can change the source access time in STAR headers, so the existing test now checks default fast-LZ4 selection and extracted content instead of compressed-byte equality.

PR flow

Standalone runner avoids redundant input copies; all runners create codec-specific tar archives, validate unique segment names, then push.

flowchart TD
  N0["StandaloneInputHandling #40;F12#41;"]:::stModified
  N1["GenerateSegment #40;F3#44; F7#44; F12#41;"]:::stUnchanged
  N2["CreateSegmentTar #40;F3#44; F7#44; F12#41;"]:::stModified
  N3["ValidateUniqueSegmentNames #40;F13#41;"]:::stAdded
  N4["PushSegments #40;F13#41;"]:::stModified
  N0 -->|"input file used"| N1
  N1 -->|"segment name used"| N2
  N2 -->|"tar paths passed"| N3
  N3 -->|"validation passed"| N4
  classDef stAdded fill:#dafbe1,stroke:#1a7f37,color:#1f2328,stroke-width:2px
  classDef stModified fill:#fff8c5,stroke:#9a6700,color:#1f2328,stroke-width:2px
  classDef stRemoved fill:#ffebe9,stroke:#cf222e,color:#1f2328,stroke-width:2px
  classDef stUnchanged fill:#f6f8fa,stroke:#656d76,color:#1f2328,stroke-width:1px

Loading

AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing

Diff evidence

• F3: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-hadoop/src/main/java/org/apache/pinot/plugin/ingestion/batch/hadoop/HadoopSegmentCreationMapper.java — before · after
• F7: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-spark-3/src/main/java/org/apache/pinot/plugin/ingestion/batch/spark3/SparkSegmentGenerationJobRunner.java — before · after
• F12: pinot-plugins/pinot-batch-ingestion/pinot-batch-ingestion-standalone/src/main/java/org/apache/pinot/plugin/ingestion/batch/standalone/SegmentGenerationJobRunner.java — before · after
• F13: pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SegmentPushUtils.java — before · after

• Regenerate PR flow

[xiangfu0]

Reviewed the diff across correctness, mixed-version/serialization compatibility, concurrency, and tests. The design is sound end-to-end: codec is selected by archive extension and TarCompressionUtils already auto-detects zstd/lz4 by magic number on untar, so generation and server-side extraction line up. The borrow-local-input optimization is correctly scoped (standalone only, gzip excluded because gzip readers unpack a sibling, path resolution mirrors LocalPinotFS.toFile, and the finally guard never deletes the borrowed source). Tests are focused unit tests with meaningful assertions.

No correctness/compatibility/concurrency issues that block merge. Only minor, non-blocking notes left inline (a codec allow-list asymmetry, a narrow push-time behavior change worth a release note, a zstd server-support prerequisite, and a small TOCTOU error-clarity idea).

Assessment: this is an approve-level review. Submitting as a Comment because the only available credential is the PR author's account, and GitHub doesn't permit approving your own PR.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 77.58621% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.76%. Comparing base (678c8b0) to head (ea29c2f).
⚠️ Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
...he/pinot/segment/local/utils/SegmentPushUtils.java	63.15%	4 Missing and 3 partials ⚠️
...gestion/batch/common/BaseSegmentPushJobRunner.java	33.33%	1 Missing and 1 partial ⚠️
...ingestion/batch/spec/SegmentGenerationJobSpec.java	75.00%	1 Missing and 1 partial ⚠️
...n/batch/standalone/SegmentGenerationJobRunner.java	90.90%	0 Missing and 1 partial ⚠️
...plugin/segmentuploader/SegmentUploaderDefault.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19898      +/-   ##
============================================
+ Coverage     69.68%   69.76%   +0.07%     
- Complexity     1491     1506      +15     
============================================
  Files          3548     3549       +1     
  Lines        235054   235130      +76     
  Branches      37702    37724      +22     
============================================
+ Hits         163804   164034     +230     
+ Misses        58586    58420     -166     
- Partials      12664    12676      +12     

Flag	Coverage Δ
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-25	69.76% <77.58%> (+0.07%)	⬆️
lane-a	100.00% <ø> (ø)
lane-b	0.00% <ø> (ø)
temurin	69.76% <77.58%> (+0.07%)	⬆️
unittests	69.76% <77.58%> (+0.07%)	⬆️
unittests1	58.93% <3.22%> (+<0.01%)	⬆️
unittests2	41.62% <77.58%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[xiangfu0]

Re-reviewed after the new push (head 38ef4bb). The delta since my last review is documentation-only and addresses my earlier feedback:

• README now has an "Upgrade notes" section covering both the duplicate-segment-name rejection and the zstd server-support prerequisite — resolves my SegmentPushUtils and Constants release-note comments.
• Constants.SEGMENT_TAR_FILE_EXTENSIONS and the setSegmentTarCompressionCodec javadoc now spell out "generation accepts gzip/zstd; discovery also consumes existing LZ4 archives" — resolves my lz4-asymmetry comment.

I re-diffed the core logic (borrow-local-input in standalone, validateUniqueSegmentNames/getSegmentName, and codec selection): it is byte-identical to the version I already verified, so no new behavior to re-check. Note the SHA-to-SHA compare also pulls in unrelated query-planner commits from a master rebase (PinotEvaluateLiteralRule, WorkerManager, EquivalentStagesFinder) — those are not part of this PR and I did not review them.

Still approve-level with no blocking issues; one optional nit left inline. Submitting as a Comment because the only available credential is the PR author's account, which GitHub won't let approve its own PR.

[gortiz]

Thanks for the detailed benchmarks. Zstd as an opt-in codec looks like a good addition, and the gzip default keeps existing deployments safe.

One consumer was missed. LaunchBackfillIngestionJobCommand.collectSegmentNames still filters on .tar.gz only (pinot-tools, around line 272). With segmentTarCompressionCodec: zstd, generatedSegments comes back empty, so the lineage entry is created with an empty segmentsTo. The tar push then uploads the .tar.zst archives anyway, because the push path now accepts them. The new segments end up outside the lineage entry, so the backfill loses its consistent-replace guarantee. Can you switch it to Constants.isSegmentTarFile and SegmentPushUtils.getSegmentNameFromTarFile, and add a test for it? SegmentUploaderDefault has the same .tar.gz filter at line 102. It is harmless today because only FileBasedSegmentWriter feeds it, but it would be good to align it too.

[xiangfu0]

Addressed the backfill consumer gap in d13bd12: collection recognizes gzip/Zstd/LZ4, skips metadata sidecars/directories, and applies the push pattern; Hadoop/Spark TAR/URI push now uses the same filtered archive selection. The default segment uploader also uses the shared archive predicate. A real-files backfill regression covers nested archives, Zstd-only selection, and local paths containing spaces.

The LZ4 change is split into #19946, duplicate validation into #19947, and usage docs into pinot-contrib/pinot-docs#1119. Gzip remains the default. Targeted JDK 25 tests, affected-module precommit checks, and independent review passed for both source changes.

[gortiz]

Thanks for splitting the LZ4 and duplicate-validation changes out and for fixing the backfill consumer. The borrowed-input path looks safe to me, and the metadata push fix is a nice catch: the old instanceof LocalPinotFS check never matched because PinotFSFactory.create returns a NoClosePinotFS wrapper, so local metadata push always copied the tar.

Approving. I left three non-blocking comments inline. Feel free to apply them here, in a follow-up, or not at all.

[xiangfu0]

Documentation follow-up merged: pinot-contrib/pinot-docs#1138 — documents segmentTarCompressionCodec, Zstandard upgrade compatibility, metadata sidecars and archive discovery/push pattern guidance.