fix: add raise_if_key_not_exists to CreateBuilder #2565
Conversation
|
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
vegarsti
changed the title
|
Can you expose this to python as well |
|
Yeah! The approach is ok? |
Yess, if you can also add one test to check if non delta keys are getting passed through and so forth. |
|
Would you mind pointing me to the relevant Python file? 😄 Is it |
Yes there! And python/src/lib.rs |
|
Thanks! Added now, let me know if it doesn't make sense. Will add the test you mentioned as well. |
python/deltalake/table.py
Outdated
| @@ -457,6 +457,7 @@ def create( | |||
| configuration: Optional[Mapping[str, Optional[str]]] = None, | |||
| storage_options: Optional[Dict[str, str]] = None, | |||
| custom_metadata: Optional[Dict[str, str]] = None, | |||
| raise_if_key_not_exists: Optional[bool] = True, | |||
There was a problem hiding this comment.
I would keep it required
There was a problem hiding this comment.
Yeah, fair! Should I move it further up in the signature, then, i.e. before the Optional ones?
There was a problem hiding this comment.
or wait, I'll still use the default True, so it should be fine
There was a problem hiding this comment.
Best to keep it aligned across :)
There was a problem hiding this comment.
Not sure what you mean by that 🤔 Had to move it further up in create_deltalake: The Rust code was failing to compile because of the ambigious ordering (it wants non-optional before optional ones).
|
BTW the order in the |
|
Added test to the Rust code now |
|
Should I squash the Rust and Python commits? |
No worries, I'll do that with the merge commit |
|
Thanks for helping so quickly! |
| @@ -196,6 +198,12 @@ impl CreateBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// Specify whether to raise an error if the table properties in the configuration are not DeltaConfigKeys | |||
| pub fn with_raise_if_not_exists(mut self, raise_if_key_not_exists: bool) -> Self { | |||
There was a problem hiding this comment.
oops @ion-elgreco I messed this up -- should have been with_raise_if_key_not_exists for consistency
There was a problem hiding this comment.
@vegarsti oops :), you can make a small follow up PR
I forgot to change the name of the builder method! Follow up from #2565.
This PR adds a
raise_if_key_not_existsflag to theCreateBuilderwhich we propagate (currently alwaystrue), such that users can choose not to fail if the configuration contains a key that is not a valid DeltaConfigKey. Also expose this in the Python binding.Closes #2564.
@ion-elgreco