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
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.apache.iceberg.TableScan;
import org.apache.iceberg.Transaction;
import org.apache.iceberg.UpdateSchema;
import org.apache.iceberg.UpdateProperties;
import org.apache.iceberg.aws.s3.S3FileIO;
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.TableIdentifier;
Expand Down Expand Up @@ -194,6 +195,18 @@ public boolean createTable(Schema schema, PartitionSpec spec, boolean overwrite)
* {
* "drop":["c1","c2"]
* }
*
* 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"]
* }

*
*
* There are more ALTER operations that can be performed according to the documentation,
Expand All @@ -205,6 +218,8 @@ public boolean alterTable(String newSchema) throws Exception {
final int OP_ADD = 1;
final int OP_DROP = 2;
final int OP_RENAME = 4;
final int OP_SET_PROP = 8;
final int OP_RM_PROP = 16;
loadTable();
UpdateSchema updateSchema = iceberg_table.updateSchema();
JSONObject schemaSpecs = new JSONObject(newSchema);
Expand Down Expand Up @@ -265,12 +280,6 @@ public boolean alterTable(String newSchema) throws Exception {
// no columns to rename, move on
}

// have we altered anything?
if (op == OP_NONE) {
System.out.println("Unrecognized ALTER operation.");
return false;
}

// confirm DROP wasn't bundled with any other ALTERs
if ((op & OP_DROP) == OP_DROP && op != OP_DROP) {
System.out.println("Cannot perform DROP along with other ALTER operations.");
Expand All @@ -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.


// check for updates to table properties
UpdateProperties updateProperties = iceberg_table.updateProperties();
try {
JSONArray setProps = schemaSpecs.getJSONArray("set_prop");
for (int i = 0; i < setProps.length(); i++) {
try {
JSONObject jo = setProps.getJSONObject(i);
String key = jo.getString("key");
String value = jo.getString("value");
updateProperties.set(key, value);
op |= OP_SET_PROP;
} catch (JSONException e) {
System.out.println("Invalid key/value property.");
return false;
}
}
} catch (JSONException e) {
// no properties to set, move on
}

try {
JSONArray rmProps = schemaSpecs.getJSONArray("rm_prop");
for (int i = 0; i < rmProps.length(); i++) {
try {
String key = rmProps.getString(i);
updateProperties.remove(key);
op |= OP_RM_PROP;
} catch (JSONException e) {
System.out.println("Invalid property key.");
return false;
}
}
} catch (JSONException e) {
// 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.


// have we altered anything?
if (op == OP_NONE) {
System.out.println("Unrecognized ALTER operation.");
return false;
}

return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ private void initializeCommands() {
Command alter = new Command("alter", "Alter a table");
alter.addOption("--help", "Show this help message and exit");
alter.addArgument("identifier", "Table or namespace identifier", true);
create.addArgument("schema", "Alter a table using this schema");
alter.addArgument("schema", "Alter a table using this schema");
m_commands.put("alter", alter);

Command drop = new Command("drop", "Drop a table or a namespace");
Expand Down