zabetak commented on code in PR #3939:
URL: https://github.com/apache/hive/pull/3939#discussion_r1073656566
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/create/CreateTableDesc.java:
##########
@@ -958,6 +940,31 @@ public Table toTable(HiveConf conf) throws HiveException {
return tbl;
}
+ public static void setColumnsAndStorePartitionTransformSpec(
Review Comment:
Consider moving the method in `DDLUtils` or `AlterTableUtils`.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/create/CreateTableDesc.java:
##########
@@ -958,6 +940,31 @@ public Table toTable(HiveConf conf) throws HiveException {
return tbl;
}
+ public static void setColumnsAndStorePartitionTransformSpec(
+ List<FieldSchema> columns, List<FieldSchema> partitionColumns,
+ HiveConf conf, Table tbl, HiveStorageHandler storageHandler)
Review Comment:
`storageHandler` can be obtained from `tbl` so we could omit this argument.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -13720,15 +13720,7 @@ ASTNode analyzeCreateTable(
}
break;
case HiveParser.TOK_TABLEPARTCOLSBYSPEC:
- List<TransformSpec> partitionTransformSpec =
- PartitionTransform.getPartitionTransformSpec(child);
-
- if (!SessionStateUtil.addResource(conf,
hive_metastoreConstants.PARTITION_TRANSFORM_SPEC,
- partitionTransformSpec)) {
- throw new SemanticException("Query state attached to Session state
must be not null. " +
- "Partition transform metadata cannot be saved.");
- }
-
Review Comment:
Since the `if` block appears in at least three places consider refactoring
it independently. It could be done as a more generic method in
`SessionStateUtil`.
```java
public static void addResourceOrThrow(Configuration conf, String key, Object
resource) {
Optional<QueryState> queryState = getQueryState(conf);
if (queryState.isPresent()) {
queryState.get().addResource(key, resource);
} else {
throw new IllegalStateException("Query state is missing; failed to add
resource for " + key);
}
}
```
Then its up to you if you want to introduce
`parseAndStorePartitionTransformSpec` or not.
##########
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:
##########
@@ -14190,11 +14200,17 @@ protected ASTNode analyzeCreateView(ASTNode ast, QB
qb, PlannerContext plannerCt
storageFormat.getSerdeProps());
}
break;
+ case HiveParser.TOK_TABLEPARTCOLSBYSPEC:
+ parseAndStorePartitionTransformSpec(child);
+ partitionTransformSpecExists = true;
+ break;
default:
assert false;
}
}
+ validateStorageFormat(storageFormat, tblProps,
partitionTransformSpecExists);
Review Comment:
In order to test completely this code path it may be useful to add some
negative tests with views and unsupported partition by clauses.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/create/CreateTableDesc.java:
##########
@@ -958,6 +940,31 @@ public Table toTable(HiveConf conf) throws HiveException {
return tbl;
}
+ public static void setColumnsAndStorePartitionTransformSpec(
Review Comment:
Consider including `table` somewhere in the method name to better reflect
its purpose.
--
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-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
