ngsg commented on code in PR #6013:
URL: https://github.com/apache/hive/pull/6013#discussion_r2272156796
##########
parser/src/java/org/apache/hadoop/hive/ql/parse/AlterClauseParser.g:
##########
@@ -104,6 +104,7 @@ alterTblPartitionStatementSuffix[boolean partition]
| alterStatementSuffixRenameCol
| alterStatementSuffixAddCol
| alterStatementSuffixDropCol
+ | alterStatementSuffixSetDefaultPartition
Review Comment:
Do we need to support queries like `ALTER TABLE tbl PARTITION (part_spec)
SET DEFAULT PARTITION TO "NA"`?
If not, I think it would be better to place
`alterStatementSuffixSetDefaultPartition` under `alterTableStatementSuffix`.
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -876,7 +877,7 @@ public DynamicPartitionCtx createDPContext(
Table table = IcebergTableUtil.getTable(conf, tableDesc.getProperties());
DynamicPartitionCtx dpCtx = new
DynamicPartitionCtx(Maps.newLinkedHashMap(),
- hiveConf.getVar(ConfVars.DEFAULT_PARTITION_NAME),
+ PartitionUtils.getDefaultPartitionName(hmsTable.getParameters(),
(HiveConf) conf),
Review Comment:
I think we can use `hiveConf` instead of `conf` here.
##########
hcatalog/core/src/main/java/org/apache/hive/hcatalog/mapreduce/DynamicPartitionFileRecordWriterContainer.java:
##########
@@ -72,7 +73,8 @@ class DynamicPartitionFileRecordWriterContainer extends
FileRecordWriterContaine
*/
public DynamicPartitionFileRecordWriterContainer(
RecordWriter<? super WritableComparable<?>, ? super Writable> baseWriter,
- TaskAttemptContext context) throws IOException, InterruptedException {
+ TaskAttemptContext context, org.apache.hadoop.hive.metastore.api.Table
tbl)
Review Comment:
Could we import `Table` and replace the qualified type name with it?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/setdefaultpartition/AlterTableSetDefaultPartitionAnalyser.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.ddl.table.setdefaultpartition;
+
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.ql.QueryState;
+import org.apache.hadoop.hive.ql.ddl.DDLSemanticAnalyzerFactory.DDLType;
+import org.apache.hadoop.hive.ql.ddl.DDLWork;
+import org.apache.hadoop.hive.ql.ddl.table.AbstractAlterTableAnalyzer;
+import org.apache.hadoop.hive.ql.exec.TaskFactory;
+import org.apache.hadoop.hive.ql.parse.ASTNode;
+import org.apache.hadoop.hive.ql.parse.HiveParser;
+import org.apache.hadoop.hive.ql.parse.SemanticException;
+
+import java.util.Map;
+
+/**
+ * Analyzer for set default partition command.
+ */
+@DDLType(types = HiveParser.TOK_ALTERTABLE_SETDEFAULTPARTITION)
+public class AlterTableSetDefaultPartitionAnalyser extends
AbstractAlterTableAnalyzer {
+
+ public AlterTableSetDefaultPartitionAnalyser(QueryState queryState) throws
SemanticException {
+ super(queryState);
+ }
+
+ @Override
+ protected void analyzeCommand(TableName tableName, Map<String, String>
partitionSpec, ASTNode command)
+ throws SemanticException {
+ String tableLevelDefaultPartitionName =
unescapeSQLString(command.getChild(0).getText());
+ AlterTableSetDefaultPartitionDesc desc = new
AlterTableSetDefaultPartitionDesc(tableName,
+ tableLevelDefaultPartitionName);
Review Comment:
Could we use an indent of 4 spaces(2 tabs) instead of 8 spaces?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/setdefaultpartition/AlterTableSetDefaultPartitionDesc.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.ddl.table.setdefaultpartition;
+
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.ql.ddl.table.AbstractAlterTableDesc;
+import org.apache.hadoop.hive.ql.ddl.table.AlterTableType;
+import org.apache.hadoop.hive.ql.parse.SemanticException;
+import org.apache.hadoop.hive.ql.plan.Explain;
+import org.apache.hadoop.hive.ql.plan.Explain.Level;
+
+/**
+ * DDL task description for ALTER TABLE ... SET DEFAULT PARTITION ... command.
+ */
+@Explain(displayName = "Set Default Partition", explainLevels = { Level.USER,
Level.DEFAULT, Level.EXTENDED })
+public class AlterTableSetDefaultPartitionDesc extends AbstractAlterTableDesc {
+ private static final long serialVersionUID = 1L;
+ private final String tableLevelDefaultPartitionName;
+
+ public AlterTableSetDefaultPartitionDesc(TableName tableName, String
tableLevelDefaultPartitionName) throws SemanticException {
+ super(AlterTableType.SETDEFAULTPARTITION, tableName, null, null, false,
false, null);
+ this.tableLevelDefaultPartitionName = tableLevelDefaultPartitionName;
+ }
+
+ @Explain(displayName = "default partition name", explainLevels = {
Level.USER, Level.DEFAULT, Level.EXTENDED })
+ public String tableLevelDefaultPartitionName() {
Review Comment:
Could we rename the method name to `getTableLevelDefaultPartitionName()`?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/setdefaultpartition/AlterTableSetDefaultPartitionDesc.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.ddl.table.setdefaultpartition;
+
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.ql.ddl.table.AbstractAlterTableDesc;
+import org.apache.hadoop.hive.ql.ddl.table.AlterTableType;
+import org.apache.hadoop.hive.ql.parse.SemanticException;
+import org.apache.hadoop.hive.ql.plan.Explain;
+import org.apache.hadoop.hive.ql.plan.Explain.Level;
+
+/**
+ * DDL task description for ALTER TABLE ... SET DEFAULT PARTITION ... command.
+ */
+@Explain(displayName = "Set Default Partition", explainLevels = { Level.USER,
Level.DEFAULT, Level.EXTENDED })
+public class AlterTableSetDefaultPartitionDesc extends AbstractAlterTableDesc {
+ private static final long serialVersionUID = 1L;
+ private final String tableLevelDefaultPartitionName;
+
+ public AlterTableSetDefaultPartitionDesc(TableName tableName, String
tableLevelDefaultPartitionName) throws SemanticException {
+ super(AlterTableType.SETDEFAULTPARTITION, tableName, null, null, false,
false, null);
+ this.tableLevelDefaultPartitionName = tableLevelDefaultPartitionName;
+ }
+
+ @Explain(displayName = "default partition name", explainLevels = {
Level.USER, Level.DEFAULT, Level.EXTENDED })
+ public String tableLevelDefaultPartitionName() {
+ return tableLevelDefaultPartitionName;
+ }
+
+ @Override
+ public boolean mayNeedWriteId() {
Review Comment:
The default bahaviour of this method is always returning `true`, so we don't
need to override it with identical logic here.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/setdefaultpartition/AlterTableSetDefaultPartitionAnalyser.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.ql.ddl.table.setdefaultpartition;
+
+import org.apache.hadoop.hive.common.TableName;
+import org.apache.hadoop.hive.ql.QueryState;
+import org.apache.hadoop.hive.ql.ddl.DDLSemanticAnalyzerFactory.DDLType;
+import org.apache.hadoop.hive.ql.ddl.DDLWork;
+import org.apache.hadoop.hive.ql.ddl.table.AbstractAlterTableAnalyzer;
+import org.apache.hadoop.hive.ql.exec.TaskFactory;
+import org.apache.hadoop.hive.ql.parse.ASTNode;
+import org.apache.hadoop.hive.ql.parse.HiveParser;
+import org.apache.hadoop.hive.ql.parse.SemanticException;
+
+import java.util.Map;
+
+/**
+ * Analyzer for set default partition command.
+ */
+@DDLType(types = HiveParser.TOK_ALTERTABLE_SETDEFAULTPARTITION)
+public class AlterTableSetDefaultPartitionAnalyser extends
AbstractAlterTableAnalyzer {
Review Comment:
Could we rename `Analyser` to use `Analyzer` as other subclasses?
##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java:
##########
@@ -1569,8 +1570,8 @@ public List<Partition> dropPartitions(String catName,
String dbName, String tblN
List<Partition> result = new ArrayList<>();
for (Pair<Integer, byte[]> pair : partExprs) {
byte[] expr = pair.getRight();
- String filter = generateJDOFilter(table, expr,
- conf.get(HiveConf.ConfVars.DEFAULT_PARTITION_NAME.varname));
+ String filter = generateJDOFilter(table, expr,
PartitionUtils.getDefaultPartitionName(table.getParameters(),
+ conf.get(HiveConf.ConfVars.DEFAULT_PARTITION_NAME.varname)));
Review Comment:
Could you restore the indentation level as before?
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/PartitionUtils.java:
##########
@@ -191,4 +192,17 @@ public static void addTablePartsOutputs(Hive db,
Set<WriteEntity> outputs, Table
}
}
}
+
+ public static String getDefaultPartitionName(Map<String, String>
tableParams, HiveConf conf) {
+ return getDefaultPartitionName(tableParams, HiveConf.getVar(conf,
HiveConf.ConfVars.DEFAULT_PARTITION_NAME));
+ }
+
+ public static String getDefaultPartitionName(Map<String, String>
tableParams, String defaultPartitionName) {
Review Comment:
Can we unify the two `getDefaultPartitionName()` into one that takes a
`Configuration`? It seems that we always have either `HiveConf` or
`Configuration` when calling it.
##########
ql/src/test/queries/clientpositive/alter_table_set_default_partition.q:
##########
@@ -0,0 +1,65 @@
+drop table if exists tbl_orc;
Review Comment:
We don't need "DROP IF EXISTS"; CliDiver cleans up all the table generated
during the tests.
##########
ql/src/test/queries/clientpositive/msck_repair_9.q:
##########
@@ -7,28 +7,31 @@ INSERT INTO tbl_x values(1, 'aaa', 12, 2);
INSERT INTO tbl_x values(2, 'bbb', 12, 3);
INSERT INTO tbl_x (id, name, month) values(3, 'ccc', 12);
-SET hive.exec.default.partition.name=ANOTHER_PARTITION;
+alter table tbl_x set default partition to 'ANOTHER_PARTITION';
INSERT INTO tbl_x (id, name, day) values(4, 'ddd', 3);
SHOW PARTITIONS tbl_x;
CREATE EXTERNAL TABLE tbl_y (id INT, name STRING) PARTITIONED BY (month INT,
day INT) stored as ORC location
'${system:test.tmp.dir}/apps/hive/warehouse/test.db/tbl_x/';
+alter table tbl_y set default partition to 'ANOTHER_PARTITION';
set hive.msck.path.validation=skip;
MSCK REPAIR TABLE tbl_y;
SHOW PARTITIONS tbl_y;
-SET hive.exec.default.partition.name=SECOND_PARTITION;
+alter table tbl_y set default partition to 'SECOND_PARTITION';
INSERT INTO tbl_y (id, name, day) values(4, 'ddd', 3);
+SHOW PARTITIONS tbl_y;
-SET hive.exec.default.partition.name=OTHER_PARTITION;
+alter table tbl_y set default partition to 'OTHER_PARTITION';
INSERT INTO tbl_y (id, name, day) values(4, 'ddd', 3);
-
SHOW PARTITIONS tbl_y;
set hive.msck.path.validation=ignore;
+
+dfs ${system:test.dfs.mkdir} -p
${system:test.tmp.dir}/apps/hive/warehouse/test.db//tbl_x/month=DEFAULT/day=DEFAULT;
Review Comment:
nit `//` -> `/`
***
What do you think about generating the dummy directory before the first msck
call? I think the first call examines the bahaviour of
`hive.msck.path.valudation=skip`, but after the patch applied, IIUC, there is
no invalid directory at that point.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -888,7 +887,7 @@ public boolean generateSqlFilterForPushdown(String catName,
String dbName, Strin
result.catName = catName;
result.filter = PartitionFilterGenerator.generateSqlFilter(catName,
dbName, tableName,
partitionKeys, tree, result.params, result.joins, dbHasJoinCastBug,
- ((defaultPartitionName == null) ? defaultPartName :
defaultPartitionName),
+ ((defaultPartitionName == null) ? getDefaultPartitionName(tableParams,
conf ) : defaultPartitionName),
Review Comment:
nit: `conf )` -> `conf)`
##########
ql/src/test/queries/clientpositive/alter_table_set_default_partition.q:
##########
@@ -0,0 +1,65 @@
+drop table if exists tbl_orc;
+
+create external table tbl_orc(a int, b string, c int) partitioned by (d int)
stored as orc;
+describe formatted tbl_orc;
+INSERT INTO tbl_orc (a, b, c) values(3, 'ccc', 12);
+show partitions tbl_orc;
+dfs -ls ${hiveconf:hive.metastore.warehouse.dir}/tbl_orc/;
+alter table tbl_orc set default partition to 'random_partition';
+describe formatted tbl_orc;
+show partitions tbl_orc;
+select * from tbl_orc;
+select count(*) from tbl_orc;
+INSERT INTO tbl_orc (a, b, c) values(4, 'ddd', 23);
+show partitions tbl_orc;
+select * from tbl_orc;
+select count(*) from tbl_orc;
+dfs -ls ${hiveconf:hive.metastore.warehouse.dir}/tbl_orc/;
+drop table if exists tbl_orc;
+
+
+create external table tbl_orc(a int, b string, c int) partitioned by (d int, e
string, f bigint) stored as orc;
Review Comment:
Could we rename this table so that each table in the qfile has a unique name?
e.g. `tbl_orc` -> `tbl_orc_multipart`
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java:
##########
@@ -3441,4 +3440,12 @@ private boolean shouldGetConstraintFromRawStore(String
catName, String dbName, S
return !shouldCacheTable(catName, dbName, tblName) || (canUseEvents &&
rawStore.isActiveTransaction())
|| !sharedCache.isTableConstraintValid(catName, dbName, tblName);
}
+
+ public static String getDefaultPartitionName(Map<String, String>
tableParams, Configuration conf) {
Review Comment:
Could we unify `CachedStore#getDefaultPartitionName` and
`HiveMetaStoreChecker#getDefaultPartitionName`?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java:
##########
@@ -3352,12 +3352,13 @@ public List<String> listPartitionNames(String catName,
String dbName, String tab
public List<String> listPartitionNames(final String catName, final String
dbName, final String tblName,
final String defaultPartName, final byte[] exprBytes,
final String order, final int maxParts) throws MetaException,
NoSuchObjectException {
- final String defaultPartitionName =
getDefaultPartitionName(defaultPartName);
+ final String defaultPartitionName =
getDefaultPartitionName(defaultPartName,
+ ensureGetMTable(catName, dbName, tblName).getParameters());
final boolean isEmptyFilter = exprBytes.length == 1 && exprBytes[0] == -1;
ExpressionTree tmp = null;
if (!isEmptyFilter) {
tmp = PartFilterExprUtil.makeExpressionTree(expressionProxy, exprBytes,
- getDefaultPartitionName(defaultPartName), conf);
+ defaultPartitionName, conf);
Review Comment:
Could you restore the indentation level as before?
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/HiveOperation.java:
##########
@@ -49,6 +49,8 @@ public enum HiveOperation {
null),
ALTERTABLE_DROPCOL("ALTERTABLE_DROPCOL", HiveParser.TOK_ALTERTABLE_DROPCOL,
new Privilege[]{Privilege.ALTER_METADATA},
null),
+ ALTERTABLE_SETDEFAULTPARTITION("ALTERTABLE_SETDEFAULTPARTITION",
HiveParser.TOK_ALTERTABLE_SETDEFAULTPARTITION, new
Privilege[]{Privilege.ALTER_METADATA},
Review Comment:
I have a minor question here: if I unerstand correctly, the new operation
can rename the default partition directory. Then would it be more appropriate
to use ALTER_DATA? What do you think about it?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java:
##########
@@ -4761,7 +4771,7 @@ protected boolean
canUseDirectSql(GetHelper<List<Partition>> ctx) throws MetaExc
// if there are more than one filter string we AND them together
initExpressionTree();
return directSql.generateSqlFilterForPushdown(table.getCatName(),
table.getDbName(), table.getTableName(),
- table.getPartitionKeys(), tree, null, filter);
+ table.getPartitionKeys(), tree, null, filter,
ctx.table.getParameters());
Review Comment:
nit: `ctx.table.getParameters()` -> `table.getParameters()`
--
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
