andygrove commented on PR #6059: URL: https://github.com/apache/datafusion-comet/pull/6059#issuecomment-5876651346
This is a light fully automated review since there are so many PRs open. 1. The `MsiTokenProvider` exemption in `blank_value_problem` (`native/core/src/parquet/objectstore/azure.rs:574`) covers the client id and tenant, but under `MsiTokenProvider` Hadoop also reads `fs.azure.account.oauth2.msi.endpoint` and `fs.azure.account.oauth2.msi.authority`, both through `getTrimmedPasswordString(key, default)` in 3.3.4, 3.4.1 and 3.4.2. A blank value there means the default IMDS endpoint and `https://login.microsoftonline.com/`. Hadoop's `abfs.md` MSI sample lists `msi.endpoint` empty next to the other two, so `spark.hadoop.fs.azure.account.oauth2.msi.endpoint=` still fails the scan with a "`fs.azure.account.oauth2.msi.endpoint` is blank" error while the driver reads fine. `WorkloadIdentityTokenProvider` defaults `msi.authority` and `token.file` the same way. Could those count as absent under the providers that default them? object_store's IMDS default is the same URL. 2. `sas_token` (`azure.rs:1005`) ranks `fs.azure.sas.<container>.<account>` above `fs.azure.sas.fixed.token`, but ABFS never reads the container-scoped key. `AbfsConfiguration.getSASTokenProvider` only looks at `fs.azure.sas.token.provider.type` and `fs.azure.sas.fixed.token`, and the container key belongs to the WASB driver (`AzureNativeFileSystemStore`). So someone who followed Example 4 in `datasources.md` and then added the `auth.type=SAS` and fixed token Hadoop needs lists files on the driver with the fixed token and reads them natively with the container token. A blank container key also fails the scan even though ABFS ignores it. Would it make sense to prefer the fixed token when it is set and keep the container key as the fallback? 3. `docs/source/user-guide/latest/datasources.md:426` still says `msi.tenant` wins when both it and `client.endpoint` are set. Under `ClientCredsTokenProvider` it is now the other way round, as line 383 and `client_creds_provider_takes_the_tenant_from_the_endpoint_only` say. Could that section say the provider class decides? -- 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]
