fix(spark): propagate per-read .option() storage credentials into path-based loadTable

[sezruby]

Problem

Path-based reads that pass storage credentials via the DataFrame .option(...) API
(e.g. an Azure SAS token) fail with an Azure Managed Identity (IMDS 169.254.169.254)
token error. Fixes #665.

LanceDataSource implements SupportsCatalogOptions, so Spark calls
extractIdentifier(options) → catalog.loadTable(identifier) and forwards only the
Identifier. LanceIdentifier kept just the location, so the path-based opens rebuilt
storage options from catalogConfig alone (empty for per-read credentials), and the
native open fell back to object_store's default Azure credential chain (MSI).

Change

• LanceIdentifier gains an optional immutable options map (back-compat 1-arg
  constructor retained). On the load path these options override catalog-level
  storage.* defaults on conflict; catalog options still fill gaps.
• LanceDataSource.extractIdentifier captures the reader options onto the identifier.
• Utils.createReadOptions gets an overload taking pathOptions, seeded before
  withCatalogDefaults to get that precedence. The dataset-URI key (path) is stripped
  from the seed so it does not leak into the native storage_options map (related to
  fix(spark): strip recognized typed options from Rust storage_options map #520, which removes the other recognized typed keys).
• Both path-based opens are covered, via a shared getPathOptions(Identifier) helper:
  • loadTableFromPath (the time-travel resolution open and the main open), and
  • tableExistsAtPath, which had the identical drop → MSI bug and is on the
    CREATE TABLE IF NOT EXISTS / overwrite path.

Credential precedence on path-based access: reader .option(...) > catalog
storage.* defaults.

Tests

• LanceIdentifierOptionsTest (unit, base module): identifier carries options
  immutably; null → empty; per-read options reach getStorageOptions(); per-read
  overrides catalog default while catalog default still fills gaps; the path key is
  not present in the storage options.
• AbfssSasReadIntegrationTest (3.5 module, @EnabledIfEnvironmentVariable-gated so CI
  skips it without creds): reads a real abfss:// dataset with the SAS supplied only via
  .option(...). Verified locally: fails on main with the MSI error
  (GET http://169.254.169.254/.../oauth2/token), passes with this change. The test
  reads the SAS from a neutral env var (LANCE_IT_SAS) so lance core's AZURE_STORAGE_*
  env fallback can't mask the connector behavior. Relates to Run test against Azure #241.

Scope and known limitations

• Read path only (+ one write wart). This PR threads .option() credentials through the
  path-based read flows (loadTableFromPath, tableExistsAtPath) and into
  stageCreateOrReplaceAtPath (so its SAS-authed existence probe and its open/commit agree).
  The remaining path-based write / drop / alter flows (createTableAtPath,
  stageCreateAtPath, stageReplaceAtPath, dropTableAtPath, resolveIdentifier, and the
  bare-String helpers lanceDatasetExists / registerExistingTable) still drop .option()
  creds. This is pre-existing behavior, not a regression; tracked as a follow-up in Thread per-read .option() storage credentials through path-based write/drop/alter flows #683 to keep
  this PR scoped to the read path.
• Typed-flag leak (fix(spark): strip recognized typed options from Rust storage_options map #520). The pathOptions channel strips only the dataset-URI (path) key
  before forwarding to the native storage_options; other recognized per-read typed flags
  (version, batch_size, block_size, ...) still pass through, same as the existing
  .option() handling. fix(spark): strip recognized typed options from Rust storage_options map #520 removes those keys; this PR does not duplicate that work.

Notes

• Complementary to fix(catalog): propagate namespace storage options into read options #522 (namespace/REST-vended creds) and fix(spark): strip recognized typed options from Rust storage_options map #520 (typed-key stripping); no
  functional overlap. If fix(catalog): propagate namespace storage options into read options #522 merges first, the path-based call sites become
  createPathBasedReadOptions(...); this patch rebases by adding the pathOptions
  overload there instead (trivial).

🤖 Generated with Claude Code

[sezruby]

Gentle ping 🙂 this one's been sitting without a reviewer. It's green (all checks pass, spotless/checkstyle clean) and mergeable.

Quick recap: path-based reads that pass storage creds via .option(...) (e.g. an Azure SAS token) were silently dropped before the native open, so Azure fell back to Managed Identity (IMDS 169.254.169.254) and failed on clusters without one. SupportsCatalogOptions forwards only the Identifier to loadTable, and LanceIdentifier didn't carry the options, so the fix threads them through (and covers tableExistsAtPath too). Verified with a real abfss read that fails on main and passes here. Fixes #665.

@geruh @LuciferYang @fangbo would any of you have a few minutes to take a look? Happy to adjust anything. Thanks!

[LuciferYang]

getPathOptions(ident) is only consumed on the read path (loadTableFromPath, tableExistsAtPath). Since extractIdentifier is shared by reads and writes, the other path-based sites that also receive the credential-carrying Identifier still pass Optional.empty() and use catalogConfig.getStorageOptions() only:

• createTableAtPath (native write driven by readOptions.getStorageOptions())
• stageCreateAtPath / stageReplaceAtPath / stageCreateOrReplaceAtPath (StagedCommitOptions.pathBased(catalogConfig.getStorageOptions(), ...))
• dropTableAtPath (Dataset.drop(uri, catalogConfig.getStorageOptions()))
• resolveIdentifier (reached on the path-based flow via alterTable; stageReplace early-returns to stageReplaceAtPath before it)
  So .option()-supplied credentials (e.g. an Azure SAS) are dropped on the write/drop/alter path-based flows and the native open falls back to the default credential chain (Azure MSI). This is pre-existing behavior, not a regression from this PR. But this PR does introduce an intra-operation inconsistency: stageCreateOrReplaceAtPath now authenticates the existence probe (fixed tableExistsAtPath) with the SAS, then opens the same dataset without it — the probe passes and the open fails, which is hard to diagnose.

Please either thread getPathOptions(ident) into all path-based open sites in this PR (for lanceDatasetExists / registerExistingTable, which take a bare String location, pass the options down from the caller), or explicitly declare the write/drop/alter paths out of scope here and file a follow-up issue.

[LuciferYang]

This strips only the "path" key. builder.fromOptions(seedOptions) then stores the whole seed map into storageOptions (parseTypedFlags only reads keys, never removes them), so per-read typed flags like version / batch_size / block_size remain in the map forwarded to the native object store via getStorageOptions(). The comment above acknowledges the cleanup is deferred to #520, but this PR widens the leak surface through the new pathOptions channel. Fine to defer — please just call out the known limitation in the PR description (or strip the recognized typed keys here too).

[LuciferYang]

This javadoc block has two bugs that both make the regression guard silently no-op:

1. The env-var usage block (this line) documents AZURE_STORAGE_SAS_TOKEN, but the test is gated by @EnabledIfEnvironmentVariable(named="LANCE_IT_SAS") and reads System.getenv("LANCE_IT_SAS"). A developer following the doc exports AZURE_STORAGE_SAS_TOKEN, LANCE_IT_SAS stays unset, and the test is silently skipped — they think the guard ran when it did not. (This is exactly what the in-body comment "Do not simplify this back to AZURE_STORAGE_SAS_TOKEN" warns against.)
2. The "Run with" command uses -pl lance-spark-base_2.12, but this test lives in lance-spark-3.5_2.12, so that module contains no such test and -Dtest=AbfssSasReadIntegrationTest matches nothing and reports success with 0 tests run.
   Please fix both: use LANCE_IT_SAS in the usage block and the run command, and change the module to -pl lance-spark-3.5_2.12.

[sezruby]

Thanks for the careful review @LuciferYang! Addressed all three in 66a723f:

1. Write/drop/alter paths dropping creds. Agreed this shouldn't balloon the PR beyond the read path. Fixed the one intra-operation inconsistency this PR introduced: stageCreateOrReplaceAtPath now threads getPathOptions(ident) into both the open and the staged commit, so they use the same credentials as the (now SAS-authed) existence probe. The remaining path-based write/drop/alter sites (and the bare-String lanceDatasetExists / registerExistingTable helpers) are declared out of scope here and tracked in Thread per-read .option() storage credentials through path-based write/drop/alter flows #683.

2. Typed-flag leak via pathOptions. Left the stripping to fix(spark): strip recognized typed options from Rust storage_options map #520 as you suggested; called out the known limitation explicitly in the PR description (Scope and known limitations).

3. Test javadoc bugs. Fixed both: usage block and run command now use LANCE_IT_SAS, and the run command targets -pl lance-spark-3.5_2.12.

All checks green, spotless/checkstyle clean.

[sezruby]

Thanks for the detailed review @LuciferYang. Pushed in 66a723f:

• stageCreateOrReplaceAtPath now threads the per-read .option() credentials into the open and the staged commit, so they match the existence probe (tableExistsAtPath) — no more "probe authenticates with the SAS, then the open falls back to the default chain." The remaining write/drop/alter path-based sites are out of scope for this read-path PR; happy to file a follow-up issue to track them.
• Test javadoc + run command now use LANCE_IT_SAS (not AZURE_STORAGE_SAS_TOKEN, which core's with_env_azure() fallback would mask) and the correct lance-spark-3.5_2.12 module.
• The typed-flag leak (version/batch_size/block_size riding the shared options map) is the pre-existing fix(spark): strip recognized typed options from Rust storage_options map #520 cleanup — left deferred, noting it now also flows through the new pathOptions channel.

CI is green across all Spark/Scala matrices. Mind another look when you have time? cc @hamersaw for maintainer visibility.

[hamersaw]

@sezruby thanks for the PR! Looking into this a bit I have some concerns about piggybacking on the LanceIdentifier to pass credentials. It seems we're doing this because by implementing SupportsCatalogOptions means we only pass an identifier implementation (ie. LanceIdentifier) between the API and actual opening. In the background we rely on the catalog to manage credentials. So essentially means that auth through the .option() is unsupported. This is the precedent across all the OSS Spark connectors I can find. The alternatives are (1) auth through a catalog (even if basically a placeholder) (2) use env vars (ie. AZURE_STORAGE_SAS_TOKEN) or (3) instance identifies (ex. MSI / IMDS) - the native default credential chain.

Is there something specific about your use-case where we can't make one of ^^^ work? A code snippet for basically an ephemeral catalog could be something like:

spark = SparkSession.builder()
    .appName("abfss-sas-read-it")
    .master("local")
    // Register a catalog named "lance" so path-based .load() routes through it,
    // and put the SAS in its storage config — where SupportsCatalogOptions expects creds.
    .config("spark.sql.catalog.lance", "org.lance.spark.LanceNamespaceSparkCatalog")
    .config("spark.sql.catalog.lance.storage.azure_storage_sas_token", sas)
    .getOrCreate();
// ...
Dataset<Row> df = spark.read().format("lance").load(uri);   // no .option() creds

[sezruby]

Thanks @hamersaw — fair framing, and you're right about the mechanics: SupportsCatalogOptions only carries the Identifier into loadTable, so per-read .option()s aren't delivered by the framework, and catalog-managed creds are the norm for catalog-routed connectors. No argument there.

The gap your three alternatives don't cover is per-read / per-dataset credentials within a single session:

• Catalog config is static — one SAS per catalog per SparkSession. Our case reads multiple Lance datasets living in different storage accounts/tenants (with short-lived SAS that rotate) in the same job; that would need a separate catalog per credential set, or reconfiguring the session between reads.
• Env var is process-global with the same limitation — and it's actually what masked this gap, since core's with_env_azure() picks up AZURE_STORAGE_SAS_TOKEN regardless of whether the connector propagated anything.
• MSI/IMDS — the target clusters have no managed identity, which is what surfaced the original failure.

So the ask isn't "auth without a catalog" in general — it's a per-read credential channel for the multi-tenant / no-managed-identity case, which mirrors the standard Spark idiom for storage creds on file sources (.option("fs.azure...", …)).

I'd split this into two questions:

1. Should per-read creds be supported at all? If serving that multi-tenant/no-MSI case isn't a direction we want, I'm happy to close this and document catalog-config as the supported path.
2. If yes, what's the cleanest mechanism? Carrying them on LanceIdentifier was the minimal way to work within SupportsCatalogOptions. I'm not attached to it — if the second credential channel is the concern, I'm open to other plumbing (e.g. delivering .option()s through the DSv2 getTable path for path-based reads). Happy to rework whichever way fits the connector's direction.

Which would you prefer? I'll adjust accordingly.

[hamersaw]

@sezruby I guess I'm leaning towards saying we don't support per-read creds. This seems to be the precedent set by all other Spark connectors using SupportsCatalogOptions. Obviously, the goal is to get your workflow working. So I do think the correct question is whether any of these options will work for you. I think we could probably solve this a little quicker without AI responses.

[sezruby]

@hamersaw Thanks, catalog-config works for our immediate case, so we'll use that for now. On whether one of the three always suffices:

1. For registered tables, a namespace/REST catalog that vends per-table storageOptions (what fix(catalog): propagate namespace storage options into read options #522 already propagates via describeTable) can give different creds on the same account that case is covered. The gap is path-based / ad-hoc reads: a bare spark.read.format("lance").load("abfss://…") has no catalog entry to hang creds on, and dynamic/short-lived SAS can't be pre-registered as catalogs or session config. Env var and MSI are account/identity-global, so they can't express it either.
2. Lance's native object_store already accepts different creds per Dataset.open (it's not authority-cached), so the capability is there underneath — the connector dropping the .option() map before the open is the only barrier.
3. Precedent is mixed, not uniform: Iceberg/Hudi don't take per-read storage .option()s (catalog/session-level), but BigQuery, Snowflake, Delta, and the built-in JDBC source do. So per-read creds aren't unusual. It's specific to the SupportsCatalogOptions routing.
4. The failure is also misleading — instead of "storage options ignored," it surfaces as an Azure MSI/IMDS error, which sends you debugging the wrong layer:

  LanceError(IO): Generic MicrosoftAzure error: Error performing token request:
  Error performing GET http://IP/metadata/identity/oauth2/token
    ?api-version=2019-08-01&resource=https%3A%2F%2Fstorage.azure.com ... after 3 retries
      at org.lance.Dataset.openNative(Native Method)
      at org.lance.spark.BaseLanceNamespaceSparkCatalog.loadTableFromPath(...)

So: catalog for now, but complex workloads read different datasets on the same account/container with distinct short-lived creds by path, and there's no way to express that through catalog/env/MSI. I'm flexible on mechanism, the identifier carry, or delivering path reads through the DSv2 getTable options path. Would you be open to supporting the per-read case or, at minimum, failing fast with a clear message instead of the MSI red herring?

--
This is written and reviewed by me — just got some help polishing and formatting 😅

[hamersaw]

Precedent is mixed, not uniform: Iceberg/Hudi don't take per-read storage .option()s (catalog/session-level), but BigQuery, Snowflake, Delta, and the built-in JDBC source do. So per-read creds aren't unusual. It's specific to the SupportsCatalogOptions routing.

I agree, it's the fact that we use SupprotsCatalogOptions that makes this difficult.

Would you be open to supporting the per-read case or, at minimum, failing fast with a clear message instead of the MSI red herring?

If this can work using the existing catalog approach, that's the cleanest solution I think. A clear error message would certainly help this!