fix: preserve case-sensitive option keys in BaseLanceNamespaceSparkCatalog - #720
fix: preserve case-sensitive option keys in BaseLanceNamespaceSparkCatalog#720Yohahaha wants to merge 2 commits into
Conversation
….initialize() CaseInsensitiveStringMap lowercases all keys when used directly, causing case-sensitive storage backend configs to fail. Use asCaseSensitiveMap() to preserve original key casing, consistent with other code paths. Closes lance-format#717
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
Yohahaha
changed the title
|
@LuciferYang @ivscheianu could you help review this PR? thank you! |
hey @Yohahaha, thanks for the fix! Looks good to me, opened a PR targeting this branch to also add a unit test, just to cover this case: Yohahaha#1 Feel free to merge it if you think it's fine. cc: @hamersaw, probably good to have a look on this |
…alog initialize (#1)
LuciferYang
left a comment
LuciferYang
left a comment
There was a problem hiding this comment.
There's one more copy of this pattern left at LanceBlobSourceContextRule.scala:58: new java.util.HashMap[String, String](relation.options) normalizes the keys the same way, then line 60 wraps them back into a CaseInsensitiveStringMap and writes them onto the relation. AbstractLanceFtsPredicateRule.scala:96 in the same package already uses rel.options.asCaseSensitiveMap(), so the two differ by that one call. The blast radius is smaller than the catalog case: it needs a blob target, a relation carrying options, and a user key with uppercase in it, and the V2Writes.mergeOptions assert only exists on Spark 4.x. Given one-PR-one-concern, a separate issue seems cleaner. Want me to file one?
|
|
||
| // Initialize the namespace with proper configuration | ||
| Map<String, String> namespaceOptions = new HashMap<>(options); | ||
| Map<String, String> namespaceOptions = new HashMap<>(options.asCaseSensitiveMap()); |
There was a problem hiding this comment.
I'm on board with the direction: namespaceOptions and storageOptions at line 208 should have been the same key shape all along. What I want to check is that this drops the normalization wholesale instead of keeping it for the keys that actually need case preserved. Two things. First, the backend matches keys exactly (properties.get("root") and k.strip_prefix("storage.") in dir.rs, strip_prefix("headers.") in rest.rs). So ROOT=/tmp now fails with Missing required property 'root', wrapped in Failed to construct namespace impl …, while Headers.x-api-key, credential_vendor.Enabled and Storage.region are dropped with no error at all. Case in the suffix alone is still fine: storage.Region works because both as_s3_options and opendal lowercase the key again. Second, this method still reads impl, parent and single_level_ns through options.get, which is case-insensitive, so you can end up with the catalog accepting a config that the backend then reports as missing. My preference is to keep the behavior this PR has and add two small things: a warn when a key differs from its canonical form only by case, scoped to the keys the connector owns (impl, parent, parent_delimiter, single_level_ns) plus the first-party prefixes, and a note in docs/src/config.md that keys are case-sensitive. I was going to suggest keeping the original keys and adding a lowercase copy, but I tested it and it does not work. opendal's Configurator::from_iter lowercases keys before handing them to the serde-derived config, and a duplicate field comes back as ConfigInvalid. On 0.57.0, the version this PR pins, passing both Region and region gives duplicate field `region` . OSS and TOS pass the user's keys straight into from_iter (options.clone() at oss.rs:68 and tos.rs:74), and S3/GCS/Azure do the same under use_opendal=true (aws.rs:122), so a doubled map turns keys that work today into hard failures, for example storage.Endpoint on S3 with use_opendal=true. COS, GooseFS and HuggingFace always go through opendal but build their own canonical config map and read exact keys, so they are unaffected. That route needs the duplicate-field problem solved first, or narrowing down to normalizing only the suffixes under the first-party prefixes (storage., credential_vendor., headers., header., tls.).
There was a problem hiding this comment.
On the other hand, keys in docs/src/config.md now have to be copied with their exact case, but the docs don't say so. storage.* at :70, root at :126 and headers.* at :241 each show one spelling only. When it goes wrong the error is Missing required property 'root', and someone looking at their own ROOT won't connect that to casing. A sentence in the docs plus the warn scoped to connector-owned keys should cover it. Worth a look at pushDownFilters and topN_push_down in select.md:245-246 while you're there: they're the two documented keys whose canonical form isn't all lowercase, they go through exact containsKey, and getting the case wrong just silently disables them. Both were already case-sensitive before this change, so that part is a pre-existing trap rather than something this PR introduces.
Yohahaha
left a comment
Yohahaha
left a comment
There was a problem hiding this comment.
Thanks @LuciferYang! Created #737 to track the same bug in LanceBlobSourceContextRule separately.
|
@Yohahaha is there any specific use-case that is failing right now? It looks like lance core is inconsistent on applying case sensitivity to options. Without a compelling reason this feels like something we should just leave to lance core to handle IMO. |
our internal storage backend needs all the storage options must be case sensitive, but lance spark pass case in-sensitive options when call namespace#connect. |
4 participants
CaseInsensitiveStringMap lowercases all keys when used directly, causing case-sensitive storage backend configs to fail. Use asCaseSensitiveMap() to preserve original key casing, consistent with other code paths.
Closes #717