mbutrovich commented on code in PR #7221:
URL: https://github.com/apache/datafusion-comet/pull/7221#discussion_r4391422263
##########
spark/src/main/scala/org/apache/comet/objectstore/AbfsAuthResolver.scala:
##########
@@ -467,6 +470,57 @@ private[comet] object AbfsAuthResolver extends Logging {
Markers.ERROR_MESSAGE -> message)
}
+ /**
+ * Why the native store cannot read `uri` under `outcome`, or None when it
can: a mechanism
+ * Hadoop named that runs Java code per request, a resolution that failed,
or resolved values
+ * the native store rejects. All are known on the driver, so the scan falls
back to Spark's own
+ * reader, which authenticates through hadoop-azure, instead of planning
natively and failing
+ * every task. For `Resolved`, native itself answers (see
[[nativeStoreError]]), so the rules
+ * live in one place.
+ */
+ def nativeFallbackReason(uri: URI, outcome: Outcome): Option[String] = {
+ // The container and host only: a password in the user info stays out of
the reason.
+ val authority = Option(uri.getRawAuthority).getOrElse("")
+ val at = authority.lastIndexOf('@')
+ val safeAuthority =
+ if (at < 0) authority
+ else authority.substring(0, at).takeWhile(_ != ':') +
authority.substring(at)
+ val location = s"${uri.getScheme}://$safeAuthority"
+ outcome match {
+ case Declined(authType, providerClass, assertionProviderClass) =>
+ val classes = providerClass.map(c => s" with provider
$c").getOrElse("") +
+ assertionProviderClass.map(c => s" and client assertion provider
$c").getOrElse("")
+ Some(
+ s"Native scan does not support ABFS authentication auth type
$authType$classes " +
+ s"for $location")
+ case resolved: Resolved =>
+ // The message names option keys, never their values.
+ nativeStoreError(s"$location/", toOptions(resolved)).map(message =>
+ s"Native scan cannot use the ABFS authentication for $location:
$message")
+ case Failed(_, message) =>
+ // Some of Hadoop's own messages quote the whole URI, user info
included.
+ val safeMessage =
+ if (authority.isEmpty) message else message.replace(authority,
safeAuthority)
+ Some(s"Hadoop ABFS authentication failed on the driver for $location:
$safeMessage")
+ case Unconfigured => None
+ }
+ }
+
+ /**
+ * Why native cannot build the store for `url` from `options`, from the same
code the scan runs.
+ * When native cannot be asked (the library is not loaded), this assumes it
can: such a build
+ * cannot run the native scan anyway.
+ */
+ private def nativeStoreError(url: String, options: Map[String, String]):
Option[String] = {
+ val (keys, values) = options.toArray.unzip
+ try Option(NativeBase.abfsStoreError(url, keys, values))
+ catch {
+ case e @ (NonFatal(_) | _: LinkageError) =>
+ logDebug(s"Could not ask native whether it can build the ABFS store
for $url", e)
+ None
+ }
Review Comment:
The doc comment justifies assuming the store builds when the library is not
loaded. That is the `LinkageError` case. `NonFatal(_)` also matches the
`CometNativeException` that
[`try_unwrap_or_throw`](https://github.com/apache/datafusion-comet/blob/beda9508f907a6b00bf94bb30c265b7b95295e65/native/jni-bridge/src/errors.rs#L1076-L1079)
throws when `create_store` panics. As I read it, the scan then plans natively
and the same panic fails every task, and the only trace on the driver is a
`logDebug`.
Should only `LinkageError` assume the store builds, with any other failure
becoming a fallback reason such as "could not ask native whether it can build
the ABFS store: ..."? Falling back is the safe direction, and the reason would
show up in the explain output. A test for each case would also help: neither
the library-not-loaded path nor a native failure is covered today.
##########
native/core/src/lib.rs:
##########
@@ -319,6 +320,35 @@ pub extern "system" fn
Java_org_apache_comet_NativeBase_icebergStorageSchemes(
})
}
+/// JNI: why the native scan cannot build its ABFS store for `url` from the
object store options
+/// `keys`/`values`, or null when it can. The driver asks this when it plans a
scan whose ABFS
+/// authentication resolved, so a store native would reject falls back to
Spark instead of every
+/// task failing. See `azure::store_error`.
+#[no_mangle]
+pub extern "system" fn Java_org_apache_comet_NativeBase_abfsStoreError(
+ env: EnvUnowned,
+ _: JClass,
+ url: JString,
+ keys: JObjectArray,
+ values: JObjectArray,
+) -> jstring {
+ try_unwrap_or_throw(&env, |env| {
+ let url: String = url.try_to_string(env)?;
+ let mut configs = HashMap::new();
+ for i in 0..keys.len(env)? {
+ let key = keys.get_element(env, i)?;
+ let key = unsafe { JString::from_raw(&*env, key.into_raw())
}.try_to_string(env)?;
+ let value = values.get_element(env, i)?;
+ let value = unsafe { JString::from_raw(&*env, value.into_raw())
}.try_to_string(env)?;
+ configs.insert(key, value);
Review Comment:
What do you think about typing the two arrays as `JObjectArray<JString>`? In
jni 0.22, `get_element` then returns a `JString`, so the two `unsafe {
JString::from_raw(...) }` casts go away. The [0.22 migration
guide](https://github.com/jni-rs/jni-rs/blob/5ae9458a4ec44c5318f37ddc7569c1d4ae8a69e7/crates/jni/docs/0.22-MIGRATION.md?plain=1#L231-L234)
gives that as the reason for the type parameter. The element type then rests
on the `String[]` declaration in `NativeBase.java`, the same way `url: JString`
already rests on its `String` declaration.
```suggestion
keys: JObjectArray<JString>,
values: JObjectArray<JString>,
) -> jstring {
try_unwrap_or_throw(&env, |env| {
let url: String = url.try_to_string(env)?;
let mut configs = HashMap::new();
for i in 0..keys.len(env)? {
let key = keys.get_element(env, i)?.try_to_string(env)?;
let value = values.get_element(env, i)?.try_to_string(env)?;
configs.insert(key, value);
}
```
--
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]
