unitycatalog / unitycatalog

Open, Multi-modal Catalog for Data & AI
https://unitycatalog.io/
Apache License 2.0
2.14k stars 318 forks source link

Updating a catalog/schema without needing to rename it #110

Closed VillePuuska closed 1 month ago

VillePuuska commented 1 month ago

Is your feature request related to a problem? Please describe.

I would like to be able to update the comment on a catalog or schema. Currently the parameter new_name is mandatory and cannot be the current name of the catalog/schema. So to just change the comment, I currently need to update the catalog/schema twice: first with a new name, then back to the old name.

Describe the solution you would like

Either make the new_name parameter optional or allow it to be the current name of the catalog/schema.

Additional context

Example commands to illustrate the problem with the CLI:

bin/uc catalog create --name test_cat

bin/uc catalog update --name test_cat --comment foo  # fails and states that new_name is required
bin/uc catalog update --name test_cat --new_name test_cat --comment foo  # fails with error ALREADY_EXISTS

# to actually update the comment, we need to rename the catalog and then name it back
bin/uc catalog update --name test_cat --new_name test_cat2 --comment foo
bin/uc catalog update --name test_cat2 --new_name test_cat --comment foo
tdas commented 1 month ago

This will be good improvement in the CLI. This is indeed a good first issue. @vikrantpuppala could you point to the code that needs updating? @VillePuuska will be interest in contributing the fix? I think the fix should simply be that "--new_name" should not mandatory, and bin/uc catalog update --name test_cat2 --comment foo should be allowed. However, bin/uc catalog update --name test_cat2 should not be allowed as there is nothing to update.

VillePuuska commented 1 month ago

I took a stab at making new_name optional for updating a catalog in the CLI and server, see #158 I really don't know Java so I may very well have missed something, but this seems to work.

@tdas I didn't yet try to make it reject trying an "empty" update, e.g. bin/uc catalog update --name unity, since atm if you call update catalog without setting a comment it sets the comment to null. If this is intended, then maybe it's worth allowing an empty update like bin/uc catalog update --name unity so you can set a comment to null. If it's not intended, then I guess that's a separate bug to fix as well. What do you think?

vikrantpuppala commented 1 month ago

Thanks for the change @VillePuuska! It looks good on a high level.

Regarding "empty" update: I don't think updating comment to null would be intended, because, there are other parameters in catalog and we wouldn't want to set all of them to null if we run bin/uc catalog update --name unity. I think at the CLI level we should handle this behaviour for the update command. I agree that it can be a separate bug but I'd like to see if it can be part of the same PR since the issues are similar and hopefully we can fix it in a way that it would be easily extendible as we add more parameters to CatalogInfo

VillePuuska commented 1 month ago

Yeah, not resetting parameters that are not provided makes sense and looks like SchemaRepository.updateSchema already works this way. I updated the draft PR #158 to include not setting the comment to null and a test for this.

I also made the CLI not allow "empty" catalog updates by throwing a CliException if it gets no parameters to update. Not sure if this is the best way to handle this and at least the error message still needs to be improved.

Let me know what you think of these changes.

VillePuuska commented 1 month ago

Catalog part of this issue is now done with #158 I'll work on getting update schema (and volume) to behave similarly in later PRs.

VillePuuska commented 1 month ago

Schema part is now done with PR #219 Still need to work on getting update volume to work similarly to close this issue.