Skip to content

fix: read each native scan partition through the object store its files live in - #6753

Open
dwsmith1983 wants to merge 18 commits into
apache:mainfrom
dwsmith1983:fix/6746-multi-store-scan-partitions
Open

dwsmith1983 wants to merge 18 commits into
apache:mainfrom
dwsmith1983:fix/6746-multi-store-scan-partitions

Conversation

@dwsmith1983

@dwsmith1983 dwsmith1983 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #6746.
Closes #6747.

Rationale for this change

The native scan reads every file of a Spark partition through one object store, built from that partition's first file, and keeps only each file's path. Spark packs small files from every root path into the same partitions, so a Parquet scan over two buckets fetched the second bucket's files from the first: wrong rows when the same key exists in both, FILE_NOT_EXIST otherwise. The native CSV scan went further and built one store from the first partition's first file for every partition, so it read the wrong bucket even when no partition mixed buckets.

What changes are included in this PR?

  • NativeConfig.objectStoreKey gives the JVM the same store identity native uses (normalize_object_store_url plus object_store_url_key, and whether the scheme is routed through libhdfs). A shared fixture table is asserted on both sides.
  • CometScanExec groups files by store once and packs each store's files separately before Spark's bin packing, so no partition mixes stores. A scan over one store packs exactly as before. A scan over several stores stays native and can end up with a different number of partitions than Spark would use; spark.sql.files.maxPartitionNum applies to each store's files separately.
  • The CSV native scan splits any partition that mixes stores, builds each partition's store from that partition's own files, and takes its partition count from the serialized partitions. On Spark 3.4 a V2 scan with one input partition reports SinglePartition, so Spark plans no exchange above it; when splitting would change that count, the scan falls back to Spark instead.
  • A native scan forwards the object store settings of every scheme it reads, not only the first file's, and translates S3-compatible alias settings per bucket. Scans over s3a and hdfs, or s3a and gs, or alias paths in several buckets, stay native. Native hashes only the settings a store reads, so one bucket keeps one cached store whatever else the scan reads.
  • The scans that fall back to Spark, with a reason: a bucket read through an S3-compatible alias together with s3, s3a or another alias, since the alias settings land in the same fs.s3a.bucket.<bucket>.* scope; an alias path with no bucket next to other S3 paths; and a bucketed table whose files span stores. The rule checks root paths early and the serde checks the listed files, so partitions stored outside the table location are covered too.
  • Native planning refuses a partition that still mixes stores, with an error naming both, instead of reading through the wrong one.
  • A declined CSV conversion now falls back to Spark's own scan. Before, the CometBatchScanExec wrapper stayed in the plan and failed to execute row-based input.
  • CometCsvNativeScanExec no longer prints its native plan in toString and explain. That text included the forwarded object store options, so credentials such as fs.s3a.secret.key showed up in explain and the SQL UI.
  • datasources.md describes how scans over more than one object store are planned.

Behaviour against 1.1: alias paths spread over several buckets, and an alias bucket next to a plain s3a bucket, fell back to Spark in 1.1 and now run natively. Scans mixing scheme families ran natively in 1.1 with only the first file's settings; they still run natively and now receive every scheme's settings.

How are these changes tested?

  • CometMultiStoreScanSuite (new, runs in CI): Parquet and CSV over two HDFS name nodes through a local test filesystem. It checks that Spark's layout mixes them and Comet's does not, that one name node keeps Spark's layout exactly, and that the CSV scan reports the partition count of its serialized partitions. It covers a CSV scan Spark plans as one partition under a global aggregate (falls back on Spark 3.4, splits under an exchange on 3.5 and later), mixed-scheme Parquet and CSV scans and a catalog partition in another scheme staying native, a scan whose scheme lists are set only as Hadoop keys staying native, the bucketed fallback with and without a partition filter, a dynamic partition pruning case, and that explain never shows a forwarded option.
  • CometScanSchemeFallbackSuite: a seeded property test for the per-store packing (no mixed partition, every file once, store and file order kept, each store packed as Spark packs it, one store unchanged), a bound on store-key comparisons that fails if grouping goes back to one pass per store, the CSV split, and a table of fallback reasons for every case above.
  • NativeConfigSuite and parquet_support.rs: the same store-key fixture on both sides, the union of options across schemes, and alias settings translated per bucket regardless of file order. A Rust test checks the store cache ignores other backends' settings.
  • Rust planner tests: a partition mixing buckets or schemes is rejected, each CSV partition gets its own store, an empty CSV partition reads no rows, and an out-of-range partition index is an error.
  • ParquetReadFromS3Suite and the new manual CsvReadFromS3Suite (MinIO): two buckets with the same and with distinct keys, mixed and separate partitions, AQE on and off, a global aggregate over a one-partition scan, dynamic partition pruning, the bucketed fallback, and alias paths in two buckets with their own settings. These reproduce both issues on main and match Spark with this change. Both MinIO suites and CometMultiStoreScanSuite were also run on the Spark 3.4 profile.

Native planning reads every file of a Spark partition through one object
store, built from the partition's first file, and keeps only each file's
path. A partition holding files from two buckets read the second bucket's
files from the first: wrong rows when a key exists in both, FILE_NOT_EXIST
otherwise. get_partitioned_files now fails with an error naming both
stores instead.

The CSV scan built one store from the first partition's first file for
every partition, so partitions in another bucket were read from the wrong
one. It now builds the store from the partition it reads, bounds-checks
the partition index, and plans an empty partition against the local
filesystem.

Part of apache#6746 and apache#6747.
…ons differ

Spark packs small files from every root path into the same partitions, so
a native Parquet or CSV scan over two buckets produced partitions that one
object store cannot serve. Files are now grouped by the store native would
use for them before Spark's bin packing (Parquet) or before serialization
(CSV). A scan over one store is packed exactly as before. A scan over
several stays native and can end up with a different number of
partitions.

NativeConfig.objectStoreKey gives the JVM the same store identity as
native, including libhdfs routing, and a shared fixture pins the two.

A scan forwards the object store settings of one scheme, so a scan whose
files span scheme families (s3a and gs, or an S3-compatible alias and
s3a) falls back to Spark, as does a bucketed scan whose files span stores.
The rule checks root paths early and the serde decides on the listed
files, which covers partitions stored outside the table location. A
declined CSV conversion now falls back to Spark's own scan instead of
leaving the Comet wrapper over row-based input.

Closes apache#6746.
Closes apache#6747.
@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading labels Oct 7, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the full 21-file diff from b56349697b786ff2ad1c1bcf6ecf45b809af5f30 to 7a24bf9ad391e1134ad91191a1b598fea6713c11. The PR is not a draft. Existing reviews, conversation comments, inline comments and snapshot threads contain no concerns to duplicate. Used review-comet-pr and the routed review-comet-expression-pr skill for the scan serde changes.

Summary

  • Prior state and problem: Parquet partitions could read different buckets through the first file’s store. CSV additionally selected that store from the first partition, potentially returning wrong rows or file-not-found errors.
  • Design approach: Identify stores consistently across JVM and native code, pack Parquet files separately per store, split mixed CSV partitions, and reject unsafe layouts defensively.
  • Correctness: Checked store identity, alias/libhdfs routing, empty CSV partitions, partition indexes, CSV task counts, bucketed fallback and dynamic pruning interactions. The focused identity and packing checks passed. No introduced P1/P2 correctness issue was identified.
  • Compatibility analysis: Compared Spark’s FilePartition, FileScan and DataSourceScanExec sources across 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. Reusing Spark’s packer preserves its per-group behavior. Spark 3.5+ partition-cap behavior now applies separately per store, as documented. No protobuf schema change is required.
  • Key design decisions: Checking both roots and listed files covers catalog partitions outside the table location. Checking static selections avoids depending on unresolved dynamic pruning. Falling back for bucketed multi-store scans preserves Spark’s bucket partitioning contract.
  • Implementation sketch: NativeConfig supplies ObjectStoreKey, CometScanUtils groups or splits files, the scan rule and serde enforce fallback conditions, and the Rust planner validates partitions and selects each CSV partition’s store. Declined CSV conversions return Spark’s executable scan.
  • Performance: Found one reproducible P2 regression. packFilesPerStore traverses all files for every store. An isolated Spark 4.1.3 packing probe with 100,000 files across 1,000 stores took approximately 2,974 ms at this head, versus 97 ms with one grouping pass producing identical output. The previous packing call took approximately 5 ms on this already-safe, one-file-per-partition workload.
  • Design: Keeping each native partition within one store is a straightforward solution that fits the existing execution model. Reusing Spark’s packing algorithm avoids maintaining another bin packer.
  • Abstraction & complexity: The shared store key is justified because raw schemes alone cannot distinguish normalized aliases and libhdfs routing. The repeated filtering inside the grouping helper adds unnecessary complexity and work. One ordered grouping pass is simpler.
  • Behavioral changes worth calling out: Compared the affected paths with latest release branch branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1. Correct bucket selection, CSV fallback and empty-partition handling are intended fixes. Additional mixed-family and bucketed fallbacks, and potentially higher partition counts, are intentional and documented. The grouping slowdown is unintended.
  • Suggested improvements: Address the performance finding by grouping once while retaining first-seen store order and file order, then packing each group. A bounded scaling check can protect this behavior. No additional issue meeting the P1/P2 reporting bar was identified.

Exact-head CI: Comet CI and CodeQL both report action_required. Only the labeling workflow passed, so CI supplies no build or test verdict.

Validation: compiled the exact changed JVM helper methods against Spark 4.1.3 with minimal unrelated-dependency stubs. Passed 24 identity fixtures, 500 randomized packing/splitting comparisons, and fallback checks. The extracted native identity fixture passed with pinned url 2.5.8. CI configuration and suite-registration checks passed. These are focused probes, not full integration tests. The native planner test build failed before tests ran because hdfs-sys could not find jni.h. Full Comet JVM, Spark SQL and MinIO suites were not run. No project source or GitHub state was changed.

pack(files)
} else {
stores
.flatMap(store => pack(keyed.collect { case (key, file) if key == store => file }))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] [P2] Group files once before packing each store. keyed.collect scans the complete file list for every distinct store, making this driver-side step O(files × stores). This also slows previously correct scans whose files already occupy separate partitions. With 100,000 files across 1,000 stores, the exact-head helper took about 2.97 seconds in a bounded probe, versus 97 ms for a single grouping pass producing identical partitions. Please collect files into groups once, preserving first-seen store order and file order, then invoke pack for each group.

Evidence: Compiled the exact-head NativeConfig and changed CometScanUtils methods against Spark 4.1.3. Constructed 100,000 PartitionedFiles with paths s3a://bucket-${i % stores}/f$i and length/file_size 10,000. Invoked Spark's actual packing implementation with maxSplitBytes and openCostInBytes both 134,217,728, yielding one file per partition before and after the PR. After two warmups, median-of-three timings were: 100 stores, base 12.76 ms / head 528.17 ms / grouping once 94.02 ms; 1,000 stores, base 4.91 ms / head 2973.80 ms / grouping once 96.60 ms. Asserted identical indexed file layouts between the head and single-grouping implementation. Probe and results: /tmp/comet6753-exact-head-7bx72sfj/probe/ScalingProbe.scala and /tmp/comet6753-exact-head-7bx72sfj/ScalingProbe-isolated.log.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please collect files into groups once, preserving first-seen store order and file order, then invoke pack for each group.

Done in e909c07. CometScanUtils groups the files by store in one pass, keeping first-seen store order and file order, and both packFilesPerStore and splitPartitionsByStore use it. The layouts are unchanged. CometScanSchemeFallbackSuite now bounds the store-key comparisons for 2000 files over 200 stores, which fails with the per-store pass, and the seeded property test checks both orders.

Pass Long bounds to range explicitly, and compute the flattened file
lists before asserting on them, so the assertion text ScalaTest renders
no longer trips the missing-interpolator lint.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes for one problem on Spark 3.4, inline on CometCsvNativeScanExec. The CSV split changes the partition count of a scan that 3.4 already planned as a single partition, so aggregates and joins above it come out wrong. Spark 3.5 and later, and the Parquet path on every version, are not affected.

The rest checks out locally (at 7a24bf9). The new Rust tests pass, and NativeConfigSuite, CometScanSchemeFallbackSuite and CometMultiStoreScanSuite pass on 4.1 and 3.4. With the Hadoop FS cache left on, libhdfs can read your fake name nodes natively, and on 4.1 the native Parquet and CSV reads over both matched Spark.

return None
}
// Native planning reads each partition through one object store.
val filePartitions = CometScanUtils.splitPartitionsByStore(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On Spark 3.4 this split can return wrong results. In 3.4, DataSourceV2ScanExecBase.outputPartitioning is SinglePartition when the scan has one input partition, so EnsureRequirements plans no exchange between the partial and final aggregate, or between the two sides of a sort merge join. When that one partition mixes stores, the split makes it two and everything above the scan runs on two partitions. I ran it on 3.4.3 with native CSV over two name nodes, using your FakeHdfsAuthorityFileSystem with the FS cache left on so libhdfs can read them. agg(count(lit(1)), sum("id")) returned [5,10] and [5,60] where Spark returns [10,70]. count() returned 5 instead of 10, and a join with the split scan on the right returned 5 of the 10 rows. With the split scan on the left, the join fails with CsvScan has no file partition 1 (1 partitions). 3.5 and later don't report SinglePartition there, so 4.1 is fine. Could the conversion fall back, or keep one partition, when the split changes the partition count and op.wrapped.outputPartitioning is not an UnknownPartitioning? That check wouldn't need a shim.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could the conversion fall back, or keep one partition, when the split changes the partition count and op.wrapped.outputPartitioning is not an UnknownPartitioning?

Done in e909c07, as a fallback: keeping one partition is not an option, since native refuses a partition that mixes stores. CometCsvNativeScanExec.convert declines with a reason when the split changes the partition count and Spark's scan reports anything but UnknownPartitioning, which on 3.4 is the one-partition SinglePartition case. Spark 3.5 and later report UnknownPartitioning there, so those scans still split with an exchange above them.

}
}

test("csv scan over two name nodes splits partitions that mix name nodes") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test passes on 3.4 even though the plan above the scan is wrong there, because it only looks at the scan. Could we add a case with a global aggregate over a scan that Spark packs into one partition, and check that Comet either falls back or keeps the partition count? It can stay plan-only like the rest of the suite.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could we add a case with a global aggregate over a scan that Spark packs into one partition, and check that Comet either falls back or keeps the partition count?

Added in e909c07. The test first checks that Spark's scan has one input partition with files on both name nodes, then expects either the fallback with its reason (Spark 3.4) or the split scan with an exchange above it (3.5 and later). It fails on 3.4 without the guard. CsvReadFromS3Suite runs the same aggregate end to end over two MinIO buckets with AQE on and off, on 3.4 and 4.1, and compares the count with Spark.

scan.relation.sparkSession.sessionState.newHadoopConfWithOptions(scan.relation.options)
// The root paths can miss a file, e.g. a catalog partition located outside the table, so
// check the listed files. The static partitions are a superset of what DPP keeps.
val multiStoreReason = CometScanUtils.multiStoreFallbackReason(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The scheme-family fallback turns off native execution for scans that 1.1 ran natively, such as a Hive table with some partitions on HDFS and some on S3. Now that files are packed per store, the only reason left is that convert forwards extractObjectStoreOptions for the first file's scheme. The keys for different families don't overlap, and native only reads the S3 and Azure ones (libhdfs and the parse_url path for gs ignore them). Could we forward the options of every scheme the scan reads, and keep the fallback only for an alias next to s3 or s3a in the same bucket, where the settings really do collide?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could we forward the options of every scheme the scan reads, and keep the fallback only for an alias next to s3 or s3a in the same bucket, where the settings really do collide?

Done in e909c07. Both scans now forward the union of extractObjectStoreOptions over one path per scheme, and per bucket for an S3-compatible alias, with each alias bucket's translated settings applied last so they win over a raw fs.s3a.bucket.<b>.* key. The fallback is now keyed on the store: an alias next to s3, s3a or another alias on the same bucket, an alias path with no bucket next to other S3 paths, and bucketed scans over several stores. So a Hive table with partitions on HDFS and S3 stays native again. Since alias settings are now translated per bucket, it also drops the older fallback for alias paths over several buckets. Native hashes only the settings a store reads (dcc61a5), so a bucket keeps one cached store whatever else the scan reads.

While checking this I found that CometCsvNativeScanExec printed its native plan, forwarded options included, in toString and explain. The same commit hides it as CometNativeScanExec already does, with a test over all three explain modes.

…scan-partitions

# Conflicts:
#	spark/src/test/scala/org/apache/comet/rules/CometScanSchemeFallbackSuite.scala
The native scan now receives the object store settings of every scheme it
reads. Hashing the whole map gave one bucket a different cache entry, and
a second HTTP client and credential fetch, depending on what else the scan
read. Each backend now hashes and receives only the keys it reads: fs.s3a.*
for S3; fs.azure.* and the comet.azure.* resolution markers for Azure; and
for every store the fs.comet.* scheme lists that decide routing.
…V partitioning

- A native scan forwards the union of the object store settings over one
  path per scheme, and per bucket for an S3-compatible alias, applying each
  alias bucket's translated settings last. Scans mixing scheme families stay
  native again.
- The fallback is keyed on the store: a bucket read through an alias next
  to s3, s3a or another alias, an alias path with no bucket next to other
  S3 paths, and bucketed scans over several stores. The older fallback for
  alias paths over several buckets is gone, since the settings are now
  translated per bucket.
- On Spark 3.4 a CSV scan with one input partition reports SinglePartition,
  so Spark plans no exchange above it. The CSV conversion falls back when
  splitting by store would change that partition count.
- Files are grouped by store in one pass before packing, instead of one
  pass over all files per store.
- CometCsvNativeScanExec no longer prints its native plan, which included
  the forwarded options and their credentials, in toString and explain.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed commit e909c077bceac3ad54863adc8352c45db37c17d8. New P1/P2 findings are detailed inline.

representatives.getOrElseUpdate((scheme, aliasBucket), uri)
}
val parts = representatives.values.toSeq.map { uri =>
val (options, aliasOptions) = objectStoreOptionsAndAliasOptions(hadoopConf, uri)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Honor libhdfs routing before translating alias options. With fs.comet.s3Compliant.schemes=blob and fs.comet.libhdfs.schemes=hdfs,blob, a scan over s3a://bucket/... and blob://bucket/... passes the fallback checks. However, this call translates fs.blob.default.endpoint into fs.s3a.bucket.bucket.endpoint, which overrides the native S3 store’s own fs.s3a.endpoint. The S3 files are consequently fetched from the alias endpoint, causing failed reads or wrong rows when that endpoint contains the same key. This regresses previously safe Parquet scans with separate file partitions and an S3 file first. Please translate alias options only when that scheme actually routes to native S3.

Evidence: Compiled the exact-head helper and the base extractor from f80042f. Using synthetic endpoints fs.s3a.endpoint=http://127.0.0.1:19001 and fs.blob.default.endpoint=http://127.0.0.1:19002, the store keys were s3://bucket and blob://bucket (libhdfs), with fallback=None. Base first-file extraction selected endpoint 19001. Head union extraction selected endpoint 19002 in both URI orders. The exact native get_config function independently confirmed that the translated per-bucket value wins. No network requests were performed. Probe source and output: /tmp/comet6753-e909-review-gnwf8u52/probe/AliasBackendProbe.scala and /tmp/comet6753-e909-review-gnwf8u52/base-head-options.log.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please translate alias options only when that scheme actually routes to native S3.

Fixed. A file whose scheme routes to libhdfs now contributes no translated alias settings to the scan's options, using the same ObjectStoreKey.isLibhdfs rule the fallback check and the native planner apply. With blob in both scheme lists, a scan over s3a://bucket/... and blob://bucket/... now forwards the same options as the s3a file alone, so the S3 store keeps fs.s3a.endpoint. NativeConfigSuite covers your case in both file orders, a libhdfs-routed alias on its own, and a native-S3 alias next to s3a, which still gets its translated endpoint.

A scheme listed in both fs.comet.s3Compliant.schemes and
fs.comet.libhdfs.schemes is read through libhdfs, but a scan that also read
s3a files of the same bucket still forwarded the alias's translated
fs.s3a.bucket.<bucket>.* settings, so the native S3 store used the alias
endpoint. Such a file now contributes no translated settings.

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @dwsmith1983. One inline comment on two Parquet assertions in CometMultiStoreScanSuite that hold by construction and would fail once #6268 lands.

assert(cometFiles.forall(_.map(storeOf).distinct.size == 1), s"Comet's: $cometFiles")
assert(cometFiles.flatten.sorted == sparkFiles.flatten.sorted)
assert(cometFiles.size != sparkFiles.size)
assert(scan.outputPartitioning.numPartitions == scan.perPartitionData.length)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At this head a non-bucketed CometNativeScanExec defines outputPartitioning as UnknownPartitioning(perPartitionData.length) (CometNativeScanExec.scala:141), so this assert and the one at line 378 hold by construction, whatever the packing does. #6268 (open, approved) changes that to UnknownPartitioning(0) and sizes execution from perPartitionData, and it merges cleanly with this branch. From reading the code I expect both asserts to fail once the two meet, whichever lands second. I have not run it. Could we drop these two Parquet asserts? The CSV ones at lines 219 and 278 still check the new CometCsvNativeScanExec.outputPartitioning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could we drop these two Parquet asserts?

Dropped both. The CSV asserts stay.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed commit 5b85671e4b46671783ab2853d39d37156048f215. New P1/P2 findings are detailed inline.

"Native Parquet scan",
roots.map(_.uri),
s3CompliantSchemes,
libhdfsSchemes,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Use the effective Hadoop configuration for this early check. With fs.comet.s3Compliant.schemes=blob and fs.comet.libhdfs.schemes=hdfs,blob supplied through core-site.xml, a non-bucketed scan over s3a://bucket/a.parquet and blob://bucket/b.parquet should remain native: the stores are native S3 and libhdfs, and alias settings are not translated. However, this libhdfsSchemes value comes from SQLConf and defaults to only hdfs. The new check therefore treats both paths as native S3, reports an alias collision, and immediately falls back to Spark. The base admitted these roots, including previously safe scans with separate file partitions. Please use NativeConfig.resolveLibhdfsSchemes(hadoopConf) here, consistently with serialization and packing, and cover Hadoop-only configuration.

Evidence: Compiled the exact-head NativeConfig and changed CometScanUtils helpers against Spark 4.1.3. A probe loaded both scheme properties from Hadoop XML while leaving SQLConf unset and exercised the rule’s configuration selection. Output: base gate declines=false; early routing=hdfs; effective routing=blob,hdfs; the head early gate returned the alias-collision reason, while the same check with effective Hadoop routing returned None. Effective keys were s3://bucket and blob://bucket (libhdfs). Source inspection confirms the returned reason causes nativeScan to return None. Reproduction: /tmp/comet6753-5b856-review-62crfd9k/src/EarlyGateProbe.scala, output in EarlyGateProbe.log. No object-store requests were made.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please use NativeConfig.resolveLibhdfsSchemes(hadoopConf) here, consistently with serialization and packing, and cover Hadoop-only configuration.

Done. The early check now reads both scheme lists from the Hadoop conf, as convert and native do. A new CometMultiStoreScanSuite test sets fs.comet.s3Compliant.schemes=blob and fs.comet.libhdfs.schemes=hdfs,blob only as Hadoop keys and checks that a scan over s3a://bucket and blob://bucket stays native, with each store's files in their own partitions.

The scheme gate above it still reads the libhdfs list from SQLConf, which predates this PR; I filed #6837 for it.

…e Hadoop conf

The rule's early check took fs.comet.libhdfs.schemes from SQLConf, while
the serde check, partition packing and native read it from the Hadoop
conf. With the scheme lists only in core-site.xml, a scan over
s3a://bucket and blob://bucket (blob routed through libhdfs) was
declined as two schemes on one store, though native reads them as two
stores.
@dwsmith1983

Copy link
Copy Markdown
Contributor Author

@andygrove the Spark 3.4 CSV case now falls back (e909c07, details in the thread on CometCsvNativeScanExec). Could you take another look? The CI run on f4a37636d needs an approval to start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scan Parquet scan / data reading bug Something isn't working

Projects

None yet

4 participants