-
Notifications
You must be signed in to change notification settings - Fork 82
fix: preserve case-sensitive option keys in BaseLanceNamespaceSparkCatalog #720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Yohahaha
wants to merge
2
commits into
lance-format:main
Choose a base branch
from
Yohahaha:worktree-fix-717
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
96 changes: 96 additions & 0 deletions
96
...base_2.12/src/test/java/org/lance/spark/BaseLanceNamespaceSparkCatalogInitializeTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| /* | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.lance.spark; | ||
|
|
||
| import org.lance.memwal.ShardingSpec; | ||
| import org.lance.spark.write.StagedCommit; | ||
|
|
||
| import org.apache.spark.sql.types.StructType; | ||
| import org.apache.spark.sql.util.CaseInsensitiveStringMap; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
||
| import java.nio.file.Path; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| public class BaseLanceNamespaceSparkCatalogInitializeTest { | ||
|
|
||
| @TempDir private Path tempDir; | ||
|
|
||
| @Test | ||
| public void testInitializePreservesCaseSensitiveOptionKeys() { | ||
| Map<String, String> rawOptions = new HashMap<>(); | ||
| rawOptions.put("impl", "dir"); | ||
| rawOptions.put("root", tempDir.toString()); | ||
| rawOptions.put("MyMixedCaseKey", "value1"); | ||
| rawOptions.put("ALLCAPS_KEY", "value2"); | ||
| rawOptions.put("camelCaseOption", "value3"); | ||
|
|
||
| CaseInsensitiveStringMap options = new CaseInsensitiveStringMap(rawOptions); | ||
|
|
||
| TestCatalog catalog = new TestCatalog(); | ||
| catalog.initialize("test", options); | ||
|
|
||
| Map<String, String> properties = catalog.getNamespaceProperties(); | ||
|
|
||
| assertTrue( | ||
| properties.containsKey("MyMixedCaseKey"), | ||
| "Should preserve mixed-case key 'MyMixedCaseKey', got keys: " + properties.keySet()); | ||
| assertTrue( | ||
| properties.containsKey("ALLCAPS_KEY"), | ||
| "Should preserve all-caps key 'ALLCAPS_KEY', got keys: " + properties.keySet()); | ||
| assertTrue( | ||
| properties.containsKey("camelCaseOption"), | ||
| "Should preserve camelCase key 'camelCaseOption', got keys: " + properties.keySet()); | ||
|
|
||
| assertEquals("value1", properties.get("MyMixedCaseKey")); | ||
| assertEquals("value2", properties.get("ALLCAPS_KEY")); | ||
| assertEquals("value3", properties.get("camelCaseOption")); | ||
| } | ||
|
|
||
| private static class TestCatalog extends BaseLanceNamespaceSparkCatalog { | ||
| @Override | ||
| public LanceDataset createDataset( | ||
| LanceSparkReadOptions readOptions, | ||
| StructType sparkSchema, | ||
| Map<String, String> initialStorageOptions, | ||
| String namespaceImpl, | ||
| Map<String, String> namespaceProperties, | ||
| boolean managedVersioning, | ||
| String fileFormatVersion, | ||
| Map<String, String> tableProperties, | ||
| ShardingSpec shardingSpec) { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public LanceDataset createStagedDataset( | ||
| LanceSparkReadOptions readOptions, | ||
| StructType sparkSchema, | ||
| Map<String, String> initialStorageOptions, | ||
| String namespaceImpl, | ||
| Map<String, String> namespaceProperties, | ||
| boolean managedVersioning, | ||
| StagedCommit stagedCommit, | ||
| String fileFormatVersion, | ||
| Map<String, String> tableProperties, | ||
| ShardingSpec shardingSpec) { | ||
| return null; | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on board with the direction:
namespaceOptionsandstorageOptionsat 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")andk.strip_prefix("storage.")indir.rs,strip_prefix("headers.")inrest.rs). SoROOT=/tmpnow fails withMissing required property 'root', wrapped inFailed to construct namespace impl …, whileHeaders.x-api-key,credential_vendor.EnabledandStorage.regionare dropped with no error at all. Case in the suffix alone is still fine:storage.Regionworks because bothas_s3_optionsand opendal lowercase the key again. Second, this method still readsimpl,parentandsingle_level_nsthroughoptions.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 indocs/src/config.mdthat 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'sConfigurator::from_iterlowercases keys before handing them to the serde-derived config, and a duplicate field comes back asConfigInvalid. On 0.57.0, the version this PR pins, passing bothRegionandregiongivesduplicate field `region`. OSS and TOS pass the user's keys straight intofrom_iter(options.clone()atoss.rs:68andtos.rs:74), and S3/GCS/Azure do the same underuse_opendal=true(aws.rs:122), so a doubled map turns keys that work today into hard failures, for examplestorage.Endpointon S3 withuse_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, keys in
docs/src/config.mdnow have to be copied with their exact case, but the docs don't say so.storage.*at:70,rootat:126andheaders.*at:241each show one spelling only. When it goes wrong the error isMissing required property 'root', and someone looking at their ownROOTwon't connect that to casing. A sentence in the docs plus the warn scoped to connector-owned keys should cover it. Worth a look atpushDownFiltersandtopN_push_downinselect.md:245-246while you're there: they're the two documented keys whose canonical form isn't all lowercase, they go through exactcontainsKey, 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.