Skip to content
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

Added ALTER operations to set and remove table properties #32

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

g-saransh
Copy link
Collaborator

GitHub issue link:

Problem:

Solution:

Testing:

  • Unit tests
  • Additional tests (add results below)

Documentation:

  • Documentation not needed
  • Updated README file
  • Documentation prepared (provide link below)

Comment on lines 199 to 209
* The property changes are expected in the following format:
* SETTING key/value property
* {
* "set_prop":[
* {"key":"p1","value":"v1"}
* ]
* }
* REMOVING property key
* {
* "rm_prop":["p1","p2"]
* }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest spelling out the operations names:

Suggested change
* The property changes are expected in the following format:
* SETTING key/value property
* {
* "set_prop":[
* {"key":"p1","value":"v1"}
* ]
* }
* REMOVING property key
* {
* "rm_prop":["p1","p2"]
* }
* The property changes are expected in the following format:
* SETTING key/value property
* {
* "set_property":[
* {"key":"p1","value":"v1"}
* ]
* }
* REMOVING property key
* {
* "remove_property":["p1","p2"]
* }

// no properties to remove, move on
}

updateProperties.commit();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit should be after your check of whether we altered anything. Otherwise, if some other operations were specified (or mistyped) you might get an exception that nothing was really committed.

@@ -279,6 +288,51 @@ public boolean alterTable(String newSchema) throws Exception {

// all good - commit changes
updateSchema.commit();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update table properties within the same ALTER as schema update? If yes, this commit should go below the if (op == OP_NONE) check. Also if yes, check for DROP explicitly as above.

Saransh Gupta added 3 commits September 27, 2023 19:04
* Expanded operation names
* Conditional commit of updateSchema and updateProperties to avoid empty commit exception
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants