[SPARK-58110][SQL] Extract SupportsArchiveFormat trait for archive-read data sources

[akshatshenoi-db]

What changes were proposed in this pull request?

Reading tar/zip archives of data files (gated by spark.sql.files.archive.reader.enabled, default off) is currently wired through a standalone ArchiveReader abstract class with TarArchiveReader/ZipArchiveReader subclasses; a data source obtains archive support by calling into that free-standing class from its read and inference paths.

This refactor replaces that class hierarchy with a SupportsArchiveFormat trait that a file-based data source mixes in to gain archive support, treating an archive like a directory of the entries it contains. The trait owns the shared streaming machinery -- isArchivePath, readArchiveEntries, lineIterator -- for formats whose per-file reader consumes an InputStream (CSV, JSON, XML, text, Avro).

Container selection (tar/tgz/zip) moves into a single openArchiveStream extension match, so the TarArchiveReader/ZipArchiveReader subclasses are removed. Schema inference is deliberately not on the trait -- each format owns its own -- so there is no shared inferArchiveSchema. The companion object keeps two statics for callers that have no trait instance: isArchivePath and a readArchiveEntries forwarder (for test suites and executor-side inference whose mapPartitions closures cannot capture an instance).

Class hierarchy before:

abstract class ArchiveReader
  <- TarArchiveReader
  <- ZipArchiveReader

after:

trait SupportsArchiveFormat        // mixed into CSVDataSource/JsonDataSource/XmlDataSource/TextFileFormat
object SupportsArchiveFormat       // isArchivePath, readArchiveEntries (statics for instance-less callers)

The CSV/JSON/XML data sources and TextFileFormat now extends ... with SupportsArchiveFormat and call the inherited readArchiveEntries/lineIterator -- a mechanical swap. Avro reads archives too, but via the companion-object statics rather than the trait: its executor-side inference runs in a mapPartitions closure that cannot capture a trait instance, so AvroFileFormat/AvroUtils call SupportsArchiveFormat.isArchivePath/readArchiveEntries directly.

Why are the changes needed?

The standalone-class shape made adding archive support to a new format a matter of threading calls to a free-standing class through the format's read and inference paths. Modeling "this data source can read archives" as a trait the format mixes in is a better fit: the shared streaming/localization machinery lives in one place, a format opts in by mixing in the trait and calling inherited methods, and container selection is centralized in one extension match instead of a subclass per container. This is a structural cleanup ahead of adding more archive-capable formats.

Does this PR introduce any user-facing change?

No. This is a behavior-preserving refactor; the archive read/infer semantics are unchanged.

How was this patch tested?

No behavior change, so this relies on the existing archive test suites: the ArchiveReaderSuite engine tests (isArchivePath dispatch and readArchiveEntries -- entry ordering, gzip handling, dir/dotfile skipping, lazy advance, the non-closing entry stream, cleanup, and the non-streamable-zip case) and the per-format tar/zip read suites. The call-site references were updated from ArchiveReader to SupportsArchiveFormat and the suites' test names/docs updated to match.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Opus 4.8)

[cloud-fan]

1 blocking, 1 non-blocking, 1 nit.
A clean, behavior-preserving extraction of the archive streaming engine. The one substantive issue is scope: an unrelated random-access localization surface rides along uncalled.

Design / architecture (1)

• SupportsArchiveFormat.scala:129: net-new random-access localization surface (readLocalizedEntries/localizeEntries/copyEntryToLocalFile/archiveEntryFilter) with no counterpart in the removed ArchiveReader and zero callers repo-wide — see inline.

Suggestions (1)

• The PR description lists AvroFileFormat among the classes that extends ... with SupportsArchiveFormat, but AvroFileFormat mixes in nothing (extends FileFormat with DataSourceRegister with SessionStateHelper with Logging with Serializable) and uses only the companion statics. This is correct in the code (its executor-side inference closure can't capture a trait instance) — only the description is inaccurate.

Nits: 1 minor item — ArchiveReaderSuite (class/file) still carries the removed ArchiveReader type's name though it now tests SupportsArchiveFormat; the doc-comment was updated but not the name. Consider renaming to SupportsArchiveFormatSuite.

[cloud-fan]

This random-access localization surface — readLocalizedEntries here plus localizeEntries, copyEntryToLocalFile, and the archiveEntryFilter hook (which defaults to throwing UnsupportedOperationException) — has no counterpart in the removed ArchiveReader (which was purely streaming) and no caller anywhere in the tree (grep across sql/, including tests, finds only the definitions). The PR description motivates it as "for random-access formats that need a complete file on disk (Parquet/ORC footers)", but neither Parquet nor ORC mixes in this trait, so nothing exercises this path.

Since this PR is framed as a behavior-preserving refactor, consider dropping this surface and landing it with the PR that adds the first random-access archive consumer — there it can be exercised and tested, rather than sitting as ~95 lines of untested, uncalled API that has to be maintained (and can rot) before its first use.

[akshatshenoi-db]

Left as part of this PR since Im currently working on follow up PRs for ORC, Parquet and Excel (DBR only) that need the surface, the code wont be uncalled or untested for more than a day or 2 max if thats ok

[cloud-fan]

Thanks for the follow-up context. I'd still prefer to keep this PR clean and drop the random-access surface (readLocalizedEntries/localizeEntries/copyEntryToLocalFile/archiveEntryFilter) here. Since this is framed as a behavior-preserving refactor, each commit should stand on its own, and ~95 lines of uncalled, untested API shouldn't ride along. Removing it now and re-adding it in the follow-up PR that first consumes it (where it lands exercised and tested) is cheap, so let's do that rather than merge it ahead of a consumer. Marking this blocking.

[cloud-fan]

Re-review: 2 addressed, 1 remaining, 0 new.
The Avro-description inaccuracy and the ArchiveReaderSuite -> SupportsArchiveFormatSuite rename from my prior review are both addressed. The streaming-engine extraction and the companion-static Avro path are correct.

Design / architecture (1, blocking)

• SupportsArchiveFormat.scala:129 (see my earlier thread): please drop the random-access surface -- readLocalizedEntries/localizeEntries/copyEntryToLocalFile/archiveEntryFilter -- from this PR. Thanks for the follow-up context, but since this is a behavior-preserving refactor, each commit should stand on its own, and ~95 lines of uncalled, untested API shouldn't ride along. Removing it now and re-adding it in the follow-up PR that first consumes it (where it lands exercised and tested) is cheap, so let's do that rather than merge it ahead of a consumer. Marking blocking.

[cloud-fan]

1 addressed, 0 remaining, 1 new. (1 = 1 newly introduced, 0 late catches.)

0 blocking, 0 non-blocking, 1 nit.
The prior blocking finding — the uncalled random-access localization surface — is cleanly removed by da944bc, and the extraction is a faithful, behavior-preserving relocation of the streaming engine. One new stale-comment nit from the removal.

Verification

Traced the extraction as behavior-preserving: the readArchiveEntries engine body (lazy per-entry advance, non-closing CloseShieldInputStream entry stream, task-completion cleanup, eager first-entry corrupt-archive surfacing, tgz base-stream leak guard) is unchanged from the removed ArchiveReader.readEntries; the tar/zip openArchiveStream bodies fold into one extension match with identical per-container logic; every call site is a mechanical symbol swap; and no source reference to ArchiveReader/TarArchiveReader/ZipArchiveReader survives (grep clean outside stale target/ artifacts).

Nits: 1 minor item (see inline comments).

[cloud-fan]

This says the non-streamable case is one "the zip reader documents", but that documentation went away with the ZipArchiveReader class — its scaladoc was where the trailing-data-descriptor case was described, and the new SupportsArchiveFormat documents no such case. State the fact directly instead of pointing at docs that no longer exist:

Suggested change

	* non-streamable case the zip reader documents; `ZipArchiveOutputStream` cannot produce it
	* non-streamable case for pure-streaming zip reads; `ZipArchiveOutputStream` cannot produce it

[cloud-fan]

thanks, merging to master/4.x, you can address that minor comment in your next PR

[cloud-fan]

Merge Summary:

• merged into master d8b8f39
• merged into branch-4.x c9a390c

Posted by merge_spark_pr.py