sunchao commented on code in PR #7127:
URL: https://github.com/apache/datafusion-comet/pull/7127#discussion_r4367931781
##########
spark/src/main/scala/org/apache/comet/objectstore/AbfsAuthResolver.scala:
##########
@@ -208,7 +208,7 @@ private[comet] object AbfsAuthResolver extends Logging {
hadoopConf: Configuration,
uri: URI,
handles: Try[Handles]): Outcome = {
- val secrets = ArrayBuffer[String]()
+ val secrets = ArrayBuffer[String](uriSecrets(uri): _*)
Review Comment:
[P2] Prepending URI values to `secrets` can weaken redaction of an existing
configuration credential. If a Hadoop diagnostic quotes a configured credential
that contains one of those URI values, `redact` masks the inner match first.
The full credential can no longer match, leaving its remaining characters in
the driver warning or forwarded error. Find matching spans in the original
diagnostic and mask their union.
Evidence: Verified by source inspection against the existing
`AbfsAuthResolverSuite` case for a credential the resolver never read. That
fixture establishes that Hadoop can quote the configured value in a forwarded
enum error. On the base, `recordSkippedSecrets` supplies the complete value for
masking. At the head it appends that value after the URI values, and the
`foldLeft` at line 483 searches the already-modified text.
##########
spark/src/main/scala/org/apache/comet/objectstore/NativeConfig.scala:
##########
@@ -132,6 +132,19 @@ object NativeConfig {
rawAuthority.substring(rawAuthority.lastIndexOf('@') + 1)
}
+ /** As in native, the user ends at the first `:` and the host follows the
last `@`. */
+ private def authorityWithoutPassword(uri: URI): String = {
+ val rawAuthority = Option(uri.getRawAuthority).getOrElse("")
+ val at = rawAuthority.lastIndexOf('@')
+ val user = rawAuthority.take(at).takeWhile(_ != ':')
Review Comment:
[P2] For an ordinary `container@host` ABFS URI, `getRawAuthority` already
returns the required authority. This helper slices the container and host and
rebuilds the same string for every file during fallback checking and again for
each split during partition packing. Return `rawAuthority` directly when the
container is nonempty and no colon precedes the final `@`. A host port then
keeps its existing handling.
Evidence: On JDK 21 and Spark 4.1.3, I ran `multiStoreFallbackReason` and
`packFilesPerStore` with Spark’s `FilePartition.getFilePartitions` on 100,000
distinct password-free ABFS files. After six warmups, median driver-thread
allocation over six passes was 89.35 MB on the base and 127.75 MB on the head.
Median thread CPU rose from 117.5 ms to 131.9 ms, with the same files assigned
to partitions.
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -620,14 +622,13 @@ fn uses_azure_container_key(scheme: &str) -> bool {
|| scheme.starts_with("abfss+comet-")
}
-pub(crate) fn object_store_authority(url: &Url) -> &str {
- let start = if uses_azure_container_key(url.scheme()) {
+pub(crate) fn object_store_authority(url: &Url) -> Cow<'_, str> {
+ if uses_azure_container_key(url.scheme()) {
// ABFS URLs encode the container in the userinfo.
- url::Position::BeforeUsername
+ authority_without_password(url)
Review Comment:
[P2] An encrypted Parquet read can fail when Spark lists a file with a
password-bearing ABFS authority. `CometFileKeyUnwrapper` registers the
retriever under the original `relation.inputFiles` URI. Native Parquet now
rebuilds the callback URI with the password-free authority, so `getKey` misses
the entry and aborts the read. Use the same canonical ABFS file key for
registration and lookup while continuing to pass the original Hadoop `Path` to
the decryption factory.
Evidence: A local Spark 4.1.3 `_spark_metadata` fixture listed
`abfss://data:[email protected]/record.parquet`. Shared-key
resolution and the encryption configuration check both passed. With callback
paths built from each revision, the production Java unwrapper returned the
fixture key on the base and threw `ParquetCryptoRuntimeException` with `Failed
to find DecryptionKeyRetriever` on the head. `abfs` reproduced the same miss.
Password-free controls passed on both revisions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
