Fix table properties case sensitivity in CreateDeltaTable #1292
Conversation
Fixes delta-io#1291 Signed-off-by: Florent Delannoy <[email protected]>
| @@ -317,7 +318,8 @@ case class CreateDeltaTableCommand( | |||
| path, tableDesc.partitionColumnNames, existingMetadata.partitionColumns) | |||
| } | |||
|
|
|||
| if (tableDesc.properties.nonEmpty && tableDesc.properties != existingMetadata.configuration) { | |||
| if (tableDesc.properties.nonEmpty && | |||
There was a problem hiding this comment.
what happens if I specify some of my table properties with correct capitalization and others lower-case? won't this fail when it should pass?
There was a problem hiding this comment.
Hey @allisonport-db ! Sorry about the delay replying here!
This comparison is case-insensitive, so both correctly capitalised table properties and lower-case properties will work, as long as they match the case-insensitively match the existing table properties.
There was a problem hiding this comment.
@Pluies please add tests for this change. you can add them to DeltaTableCreationTests.scala.
|
ping @allisonport-db , so that this doesn't fall off the radar :) Let me know if there's anything else I can do to help this moving forward 👍 |
|
Can you please update the PR description? |
What about the PR description needs updating? |
|
@vegarsti - I probably commented on the wrong PR :P Terribly sorry, so many to review aha! |
Sorry I missed this! Can you add the tests @scottsand-db mentioned? |
|
Indeed, I'm on it! I've been having some issues getting the tests to run but they're finally there, just need to implement them now :) |
|
Awesome thanks |
|
^ Update on the above, my priorities have shifted and I won't have time to add the proper tests or follow-up on comments here. Happy to leave this PS as open for someone else to take over, or close it for clarity 🙏 |
Description
Resolves #1291
How was this patch tested?
Tested by locally running the patched Delta library and confirming this fixes the issue at hand
Does this PR introduce any user-facing changes?
No