You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/07/14 13:12:28 UTC
[GitHub] [hive] marton-bod opened a new pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
marton-bod opened a new pull request #2475:
URL: https://github.com/apache/hive/pull/2475
### What changes were proposed in this pull request?
Use `REPLACE COLUMNS` only for dropping one or more columns. If there's any column reorder, column rename, adding columns, changing column type or changing column comment, we throw an exception.
### Why are the changes needed?
Limit the damage the user can do with this command :)
### Does this PR introduce _any_ user-facing change?
Yes, limitation of the REPLACE command (but only for Iceberg tables; Hive tables are unaffected).
### How was this patch tested?
unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #2475:
URL: https://github.com/apache/hive/pull/2475#discussion_r680971134
##########
File path: iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -507,35 +506,38 @@ private void handleAddColumns(org.apache.hadoop.hive.metastore.api.Table hmsTabl
}
private void handleReplaceColumns(org.apache.hadoop.hive.metastore.api.Table hmsTable) throws MetaException {
- HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsTable.getSd().getCols(),
- HiveSchemaUtil.convert(icebergTable.schema()), true);
- if (!schemaDifference.isEmpty()) {
- updateSchema = icebergTable.updateSchema();
- } else {
- // we should get here if the user restated the exactly the existing columns in the REPLACE COLUMNS command
- LOG.info("Found no difference between new and old schema for ALTER TABLE REPLACE COLUMNS for" +
- " table: {}. There will be no Iceberg commit.", hmsTable.getTableName());
- return;
- }
+ List<FieldSchema> hmsCols = hmsTable.getSd().getCols();
+ List<FieldSchema> icebergCols = HiveSchemaUtil.convert(icebergTable.schema());
+ HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsCols, icebergCols, true);
- for (FieldSchema droppedCol : schemaDifference.getMissingFromFirst()) {
- updateSchema.deleteColumn(droppedCol.getName());
+ // if there are columns dropped, let's remove them from the iceberg schema as well so we can compare the order
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
+ schemaDifference.getMissingFromFirst().forEach(icebergCols::remove);
}
- for (FieldSchema addedCol : schemaDifference.getMissingFromSecond()) {
- updateSchema.addColumn(
- addedCol.getName(),
- HiveSchemaUtil.convert(TypeInfoUtils.getTypeInfoFromTypeString(addedCol.getType())),
- addedCol.getComment()
- );
- }
+ Pair<String, Optional<String>> outOfOrder = HiveSchemaUtil.getFirstOutOfOrderColumn(
+ hmsCols, icebergCols, ImmutableMap.of());
- for (FieldSchema updatedCol : schemaDifference.getTypeChanged()) {
- updateSchema.updateColumn(updatedCol.getName(), getPrimitiveTypeOrThrow(updatedCol), updatedCol.getComment());
+ // limit the scope of this operation to only dropping columns
+ if (!schemaDifference.getMissingFromSecond().isEmpty() || !schemaDifference.getTypeChanged().isEmpty() ||
+ !schemaDifference.getCommentChanged().isEmpty() || outOfOrder != null) {
+ throw new MetaException("Unsupported operation to use REPLACE COLUMNS for adding a column, changing a " +
+ "column type, column comment or reordering columns. Only use REPLACE COLUMNS for dropping columns. " +
+ "For the other operations, consider using the ADD COLUMNS or CHANGE COLUMN commands.");
}
- for (FieldSchema updatedCol : schemaDifference.getCommentChanged()) {
- updateSchema.updateColumnDoc(updatedCol.getName(), updatedCol.getComment());
+ // check if there were any column drops
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
Review comment:
Ah I now see, you also allow no-op to happen. Wouldn't we want to restrict users from doing that too (there were no drops after all)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2475:
URL: https://github.com/apache/hive/pull/2475#discussion_r681003891
##########
File path: iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -309,9 +309,8 @@ public void commitAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable
}
@Override
- public void rollbackAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable, EnvironmentContext context)
- throws MetaException {
- if (Boolean.valueOf(context.getProperties().getOrDefault(MIGRATE_HIVE_TO_ICEBERG, "false"))) {
+ public void rollbackAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable, EnvironmentContext context) {
+ if (Boolean.parseBoolean(context.getProperties().getOrDefault(MIGRATE_HIVE_TO_ICEBERG, "false"))) {
Review comment:
Not much :) It was a checkstyle warning I think, that's why I made this change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod merged pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
marton-bod merged pull request #2475:
URL: https://github.com/apache/hive/pull/2475
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod commented on a change in pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
marton-bod commented on a change in pull request #2475:
URL: https://github.com/apache/hive/pull/2475#discussion_r681561292
##########
File path: iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -507,35 +506,38 @@ private void handleAddColumns(org.apache.hadoop.hive.metastore.api.Table hmsTabl
}
private void handleReplaceColumns(org.apache.hadoop.hive.metastore.api.Table hmsTable) throws MetaException {
- HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsTable.getSd().getCols(),
- HiveSchemaUtil.convert(icebergTable.schema()), true);
- if (!schemaDifference.isEmpty()) {
- updateSchema = icebergTable.updateSchema();
- } else {
- // we should get here if the user restated the exactly the existing columns in the REPLACE COLUMNS command
- LOG.info("Found no difference between new and old schema for ALTER TABLE REPLACE COLUMNS for" +
- " table: {}. There will be no Iceberg commit.", hmsTable.getTableName());
- return;
- }
+ List<FieldSchema> hmsCols = hmsTable.getSd().getCols();
+ List<FieldSchema> icebergCols = HiveSchemaUtil.convert(icebergTable.schema());
+ HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsCols, icebergCols, true);
- for (FieldSchema droppedCol : schemaDifference.getMissingFromFirst()) {
- updateSchema.deleteColumn(droppedCol.getName());
+ // if there are columns dropped, let's remove them from the iceberg schema as well so we can compare the order
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
+ schemaDifference.getMissingFromFirst().forEach(icebergCols::remove);
}
- for (FieldSchema addedCol : schemaDifference.getMissingFromSecond()) {
- updateSchema.addColumn(
- addedCol.getName(),
- HiveSchemaUtil.convert(TypeInfoUtils.getTypeInfoFromTypeString(addedCol.getType())),
- addedCol.getComment()
- );
- }
+ Pair<String, Optional<String>> outOfOrder = HiveSchemaUtil.getFirstOutOfOrderColumn(
+ hmsCols, icebergCols, ImmutableMap.of());
- for (FieldSchema updatedCol : schemaDifference.getTypeChanged()) {
- updateSchema.updateColumn(updatedCol.getName(), getPrimitiveTypeOrThrow(updatedCol), updatedCol.getComment());
+ // limit the scope of this operation to only dropping columns
+ if (!schemaDifference.getMissingFromSecond().isEmpty() || !schemaDifference.getTypeChanged().isEmpty() ||
+ !schemaDifference.getCommentChanged().isEmpty() || outOfOrder != null) {
+ throw new MetaException("Unsupported operation to use REPLACE COLUMNS for adding a column, changing a " +
+ "column type, column comment or reordering columns. Only use REPLACE COLUMNS for dropping columns. " +
+ "For the other operations, consider using the ADD COLUMNS or CHANGE COLUMN commands.");
}
- for (FieldSchema updatedCol : schemaDifference.getCommentChanged()) {
- updateSchema.updateColumnDoc(updatedCol.getName(), updatedCol.getComment());
+ // check if there were any column drops
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
Review comment:
As discussed offline, let's throw an exception for no-ops so that users are aware they did not change anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #2475:
URL: https://github.com/apache/hive/pull/2475#discussion_r680969086
##########
File path: iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -507,35 +506,38 @@ private void handleAddColumns(org.apache.hadoop.hive.metastore.api.Table hmsTabl
}
private void handleReplaceColumns(org.apache.hadoop.hive.metastore.api.Table hmsTable) throws MetaException {
- HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsTable.getSd().getCols(),
- HiveSchemaUtil.convert(icebergTable.schema()), true);
- if (!schemaDifference.isEmpty()) {
- updateSchema = icebergTable.updateSchema();
- } else {
- // we should get here if the user restated the exactly the existing columns in the REPLACE COLUMNS command
- LOG.info("Found no difference between new and old schema for ALTER TABLE REPLACE COLUMNS for" +
- " table: {}. There will be no Iceberg commit.", hmsTable.getTableName());
- return;
- }
+ List<FieldSchema> hmsCols = hmsTable.getSd().getCols();
+ List<FieldSchema> icebergCols = HiveSchemaUtil.convert(icebergTable.schema());
+ HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsCols, icebergCols, true);
- for (FieldSchema droppedCol : schemaDifference.getMissingFromFirst()) {
- updateSchema.deleteColumn(droppedCol.getName());
+ // if there are columns dropped, let's remove them from the iceberg schema as well so we can compare the order
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
+ schemaDifference.getMissingFromFirst().forEach(icebergCols::remove);
}
- for (FieldSchema addedCol : schemaDifference.getMissingFromSecond()) {
- updateSchema.addColumn(
- addedCol.getName(),
- HiveSchemaUtil.convert(TypeInfoUtils.getTypeInfoFromTypeString(addedCol.getType())),
- addedCol.getComment()
- );
- }
+ Pair<String, Optional<String>> outOfOrder = HiveSchemaUtil.getFirstOutOfOrderColumn(
+ hmsCols, icebergCols, ImmutableMap.of());
- for (FieldSchema updatedCol : schemaDifference.getTypeChanged()) {
- updateSchema.updateColumn(updatedCol.getName(), getPrimitiveTypeOrThrow(updatedCol), updatedCol.getComment());
+ // limit the scope of this operation to only dropping columns
+ if (!schemaDifference.getMissingFromSecond().isEmpty() || !schemaDifference.getTypeChanged().isEmpty() ||
+ !schemaDifference.getCommentChanged().isEmpty() || outOfOrder != null) {
+ throw new MetaException("Unsupported operation to use REPLACE COLUMNS for adding a column, changing a " +
+ "column type, column comment or reordering columns. Only use REPLACE COLUMNS for dropping columns. " +
+ "For the other operations, consider using the ADD COLUMNS or CHANGE COLUMN commands.");
}
- for (FieldSchema updatedCol : schemaDifference.getCommentChanged()) {
- updateSchema.updateColumnDoc(updatedCol.getName(), updatedCol.getComment());
+ // check if there were any column drops
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
Review comment:
This condition is already checked at line 518. Also - if we ever got here, shouldn't it mean there were cols dropped?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2475:
URL: https://github.com/apache/hive/pull/2475#discussion_r680976148
##########
File path: iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -309,9 +309,8 @@ public void commitAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable
}
@Override
- public void rollbackAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable, EnvironmentContext context)
- throws MetaException {
- if (Boolean.valueOf(context.getProperties().getOrDefault(MIGRATE_HIVE_TO_ICEBERG, "false"))) {
+ public void rollbackAlterTable(org.apache.hadoop.hive.metastore.api.Table hmsTable, EnvironmentContext context) {
+ if (Boolean.parseBoolean(context.getProperties().getOrDefault(MIGRATE_HIVE_TO_ICEBERG, "false"))) {
Review comment:
What is the difference here?
```
public static Boolean valueOf(String var0) {
return parseBoolean(var0) ? TRUE : FALSE;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] szlta commented on a change in pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #2475:
URL: https://github.com/apache/hive/pull/2475#discussion_r680969086
##########
File path: iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergMetaHook.java
##########
@@ -507,35 +506,38 @@ private void handleAddColumns(org.apache.hadoop.hive.metastore.api.Table hmsTabl
}
private void handleReplaceColumns(org.apache.hadoop.hive.metastore.api.Table hmsTable) throws MetaException {
- HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsTable.getSd().getCols(),
- HiveSchemaUtil.convert(icebergTable.schema()), true);
- if (!schemaDifference.isEmpty()) {
- updateSchema = icebergTable.updateSchema();
- } else {
- // we should get here if the user restated the exactly the existing columns in the REPLACE COLUMNS command
- LOG.info("Found no difference between new and old schema for ALTER TABLE REPLACE COLUMNS for" +
- " table: {}. There will be no Iceberg commit.", hmsTable.getTableName());
- return;
- }
+ List<FieldSchema> hmsCols = hmsTable.getSd().getCols();
+ List<FieldSchema> icebergCols = HiveSchemaUtil.convert(icebergTable.schema());
+ HiveSchemaUtil.SchemaDifference schemaDifference = HiveSchemaUtil.getSchemaDiff(hmsCols, icebergCols, true);
- for (FieldSchema droppedCol : schemaDifference.getMissingFromFirst()) {
- updateSchema.deleteColumn(droppedCol.getName());
+ // if there are columns dropped, let's remove them from the iceberg schema as well so we can compare the order
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
+ schemaDifference.getMissingFromFirst().forEach(icebergCols::remove);
}
- for (FieldSchema addedCol : schemaDifference.getMissingFromSecond()) {
- updateSchema.addColumn(
- addedCol.getName(),
- HiveSchemaUtil.convert(TypeInfoUtils.getTypeInfoFromTypeString(addedCol.getType())),
- addedCol.getComment()
- );
- }
+ Pair<String, Optional<String>> outOfOrder = HiveSchemaUtil.getFirstOutOfOrderColumn(
+ hmsCols, icebergCols, ImmutableMap.of());
- for (FieldSchema updatedCol : schemaDifference.getTypeChanged()) {
- updateSchema.updateColumn(updatedCol.getName(), getPrimitiveTypeOrThrow(updatedCol), updatedCol.getComment());
+ // limit the scope of this operation to only dropping columns
+ if (!schemaDifference.getMissingFromSecond().isEmpty() || !schemaDifference.getTypeChanged().isEmpty() ||
+ !schemaDifference.getCommentChanged().isEmpty() || outOfOrder != null) {
+ throw new MetaException("Unsupported operation to use REPLACE COLUMNS for adding a column, changing a " +
+ "column type, column comment or reordering columns. Only use REPLACE COLUMNS for dropping columns. " +
+ "For the other operations, consider using the ADD COLUMNS or CHANGE COLUMN commands.");
}
- for (FieldSchema updatedCol : schemaDifference.getCommentChanged()) {
- updateSchema.updateColumnDoc(updatedCol.getName(), updatedCol.getComment());
+ // check if there were any column drops
+ if (!schemaDifference.getMissingFromFirst().isEmpty()) {
Review comment:
This condition is already checked at line 514. Also - if we ever got here, shouldn't it mean there were cols dropped?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] marton-bod merged pull request #2475: HIVE-25328: Limit scope of REPLACE COLUMNS for Iceberg tables
Posted by GitBox <gi...@apache.org>.
marton-bod merged pull request #2475:
URL: https://github.com/apache/hive/pull/2475
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org