mbutrovich commented on code in PR #7221:
URL: https://github.com/apache/datafusion-comet/pull/7221#discussion_r4383763946
##########
spark/src/main/scala/org/apache/comet/objectstore/AbfsAuthResolver.scala:
##########
@@ -467,6 +469,42 @@ 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 built-in OAuth provider
with no
+ * `object_store` counterpart, or a resolution that failed. 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.
+ */
+ 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"
+ def unsupported(mechanism: String): String =
+ s"Native scan does not support ABFS authentication $mechanism for
$location"
+ 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(unsupported(s"auth type $authType$classes"))
+ case Resolved(
+ AuthTypes.OAUTH,
+ Some(c @ (Providers.REFRESH_TOKEN | Providers.USER_PASSWORD)),
+ _) =>
Review Comment:
> The check is conservative where `java.net.URI` and the native URL parser
could disagree: a percent escape, a backslash or a dot segment falls back
rather than being guessed at.
Should the fallback reason say so too? An endpoint like
`https://h/a/%2e%2e/t/oauth2/token` is `https` and has a tenant segment, but it
fails the check at [line
535](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/spark/src/main/scala/org/apache/comet/objectstore/AbfsAuthResolver.scala#L535).
The
[reason](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/spark/src/main/scala/org/apache/comet/objectstore/AbfsAuthResolver.scala#L504-L506)
then tells the user it is "not an https URL with a tenant path segment", which
sends them looking for the wrong problem. The native store builds for the
percent escape and backslash cases, per the [`oauth_endpoint_parts`
test](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/native/core/src/parquet/objectstore/azure.rs#L982-L984).
Something like "an endpoint that is not an https URL with a tenant path
segment and no percent escape
, backslash or dot segment" would match the check.
[`datasources.md`](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/docs/source/user-guide/latest/datasources.md?plain=1#L484)
could list these cases next to the `http://` and no-tenant ones.
> `nativeFallbackReason` now declines a `Resolved`
`ClientCredsTokenProvider` whose endpoint is not `https` or has no tenant
segment, using the same tenant rule as `oauth_endpoint_parts`
As I read it, nothing keeps the two copies of that rule in step. The [test
table](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/spark/src/test/scala/org/apache/comet/objectstore/NativeConfigSuite.scala#L922-L923)
is copied from the `azure.rs` cases, and the new [`azure.rs` doc
comment](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/native/core/src/parquet/objectstore/azure.rs#L40-L43)
says scans never reach these errors. If `oauth_endpoint_parts` later rejects a
shape that `clientCredsEndpointUsable` accepts, the scan plans natively and
every task fails again, and neither suite notices.
Could the driver ask native instead of reimplementing the rule? There is
precedent for a planning-time JNI probe in
[`NativeBase.icebergStorageSchemes`](https://github.com/apache/datafusion-comet/blob/5ee970e6c528544e2a528b01ea7f4126abd2e1ac/spark/src/main/java/org/apache/comet/NativeBase.java#L403).
A probe that runs `resolved_settings` plus an explicit `https` check would
also give native ABFS writes a clear error up front instead of the `BadScheme`
on each token request you saw. If that's more than this PR should take on, what
do you think about one fixture file that both the Rust test and the Scala test
read?
--
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]
