[GH-3128] Fix geotiff.metadata partition values, scan identity, and option handling#3129
Merged
Merged
Conversation
… and option handling Apply to geotiff.metadata the fixes that hardened the same code patterns in netcdf.metadata (apacheGH-3115): - Per-file partition values: a bin-packed partition wrapped one reader with the first file's partitionValues, so files from different Hive partitions reported the wrong partition columns. Each file's reader is now wrapped with its own values. - Scan equality and hashing include options and pushedLimit; FileScan's concrete equals suppressed the case-class versions, letting exchange or subquery reuse serve a limited scan's result for an unlimited one. - fallbackFileFormat returns a stub FileFormat that fails on instantiation with a descriptive error, so catalog DDL (CREATE TABLE ... USING, with or without an explicit schema) no longer throws a NullPointerException or leaves an unusable table. - Directory defaults never override explicit options: presence checks are case-insensitive, recursiveFileLookup=false keeps Hive-style partition discovery, and the GeoTIFF extension filter also applies to multi-directory loads. - A user-supplied pathGlobFilter disables the glob-path rewrite so Spark applies both the path glob and the filter natively. Adds 7 tests (19 total) mirroring the netcdf.metadata regression coverage; documents the directory-load defaults and unsupported catalog operations.
…tory loads, force bin-packing in tests - Directory-scan defaults (recursiveFileLookup and the extension pathGlobFilter) now apply only when every supplied root is a directory. Spark applies pathGlobFilter to all roots, so a mixed load(directory, explicitFile) previously dropped an explicitly named file whose name did not match the extension filter. Applied to both geotiff.metadata and netcdf.metadata, which share this logic. - The Hive partition-value regression tests now force deterministic bin-packing (openCostInBytes=0, minPartitionNum=1, maxPartitionBytes=1g) and assert a single Spark partition before checking per-file partition values; under the default local configuration each file received its own partition and the old broken implementation also passed. - Adds mixed directory-and-file load tests (with an extensionless explicit file) to both suites.
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.
Did you read the Contributor Guide?
Is this PR related to a ticket?
[GH-XXX] my subject. Closes Fix geotiff.metadata defects found during the netcdf.metadata reviews #3128What changes were proposed in this PR?
Apply to
geotiff.metadatathe fixes that hardened the same code patterns innetcdf.metadataduring the #2829 reviews.netcdf.metadatawas originally modeled ongeotiff.metadata, so these defects exist identically in the GeoTIFF package on master.Correctness (silent wrong results)
Per-file partition values. A bin-packed partition wrapped one reader in a single
PartitionReaderWithPartitionValuesusingpartition.files.head.partitionValues, so files from different Hive partitions (e.g.year=2020/,year=2021/) all reported the first file's partition values. Each file's reader is now wrapped with its own values (GeoTiffMetadataConcatPartitionReader; sub-readers open lazily, at most one file open at a time).Scan equality and hashing include pushdown state.
FileScan's concreteequals/hashCodesuppress the case-class-synthesized versions, sopushedLimitandoptionsdid not participate in equality. Since the scan alone enforces the pushed limit (isPartiallyPushed = falseremoves the Limit operator), exchange/subquery reuse could serve a limited scan's result for an unlimited one.equalsnow includes both fields andhashCodeis derived from every equality field.Crash
fallbackFileFormatno longer returns null.CREATE TABLE ... USING geotiff.metadatathrew aNullPointerException. The fallback is now a stubFileFormatthat fails on instantiation with a descriptiveUnsupportedOperationException, which also rejectsCREATE TABLEwith an explicit schema (that path skipsinferSchema).Behavior
Directory defaults never override explicit options. Single-directory loads forced
recursiveFileLookup=true, overriding an explicitfalseand silently disabling Hive-style partition discovery; multi-directory loads bypassed the GeoTIFF extension filter entirely. Both are now defaults only, applied for any directory root.Option presence checks are case-insensitive. Defaults were guarded with
containsKeyon a copiedHashMap, so an explicit mixed-case option (e.g.RecursiveFileLookup=false) could nondeterministically lose to the lowercase default.A glob path combined with an explicit
pathGlobFilterkeeps both constraints..load("/dir/a*.tif")withpathGlobFilter="b*.tif"previously rewrote the path and kept only one filter; the rewrite is now skipped when the user supplied a filter, so Spark applies both natively.Every fix mirrors its review-hardened counterpart in
spark/common/.../io/netcdfmetadata/, adjusted only for naming and the.tif/.tiffextensions. Documentation gains the directory-load defaults and the unsupported-catalog-operations note, mirroring the NetCDF page.How was this patch tested?
7 new tests in
geotiffMetadataTest.scala(19 total), mirroring the netcdf.metadata regression coverage:RecursiveFileLookup=false, asserting every row carries its own file's partition value even when bin-packed into one Spark partitionpathGlobFilterreturning the native empty intersection (with a glob-only sanity check)CREATE TABLE ... USING geotiff.metadatafailing with a clear error instead of an NPE, both without and with an explicit schemaFull
geotiffMetadataTest,netcdfMetadataTest, andrasteralgebraTestsuites pass on Spark 3.4/Scala 2.12 (240 tests); sources cross-compile on Spark 4.0/Scala 2.13.Did this PR include necessary documentation updates?