Skip to content

HIVE-29413: Avoid code duplication by updating getPartCols method for iceberg tables #6337

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.

Sign up for GitHub

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

Closed
ramitg254 wants to merge 2 commits into from
Closed

HIVE-29413: Avoid code duplication by updating getPartCols method for iceberg tables #6337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56b8cd6
HIVE-29413:Avoid code duplication by updating getPartCols method for …
ramitg254 Mar 10, 2026
0d0641e
updated api usage
ramitg254 Mar 23, 2026
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
5 changes: 1 addition & 4 deletions ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/show/ShowColumnsOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,7 @@ private List<FieldSchema> getColumnsByPattern() throws HiveException {

private List<FieldSchema> getCols() throws HiveException {
Table table = context.getDb().getTable(desc.getTableName());
List<FieldSchema> allColumns = new ArrayList<>();
allColumns.addAll(table.getCols());
allColumns.addAll(table.getPartCols());
return allColumns;
return new ArrayList<>(table.getAllCols());
Copy link
Copy Markdown
Member

@deniskuzZ deniskuzZ Mar 17, 2026

Choose a reason for hiding this comment

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

why do we need to clone it?

Copy link
Copy Markdown
Contributor Author

@ramitg254 ramitg254 Mar 23, 2026

Choose a reason for hiding this comment

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

It was already cloning it so I just replaced

List<FieldSchema> allColumns = new ArrayList<>();
allColumns.addAll(table.getCols());
allColumns.addAll(table.getPartCols());

with
new ArrayList<>(table.getAllCols());

}

private Matcher getMatcher() {
Expand Down
4 changes: 1 addition & 3 deletions .../java/org/apache/hadoop/hive/ql/ddl/table/info/desc/formatter/TextDescTableFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,7 @@ private void addPartitionData(DataOutputStream out, HiveConf conf, String column
List<FieldSchema> partitionColumns = null;
// TODO (HIVE-29413): Refactor to a generic getPartCols() implementation
if (table.isPartitioned()) {
partitionColumns = table.hasNonNativePartitionSupport() ?
table.getStorageHandler().getPartitionKeys(table) :
table.getPartCols();
partitionColumns = table.getSupportedPartCols();
Copy link
Copy Markdown
Member

@deniskuzZ deniskuzZ Mar 17, 2026

Choose a reason for hiding this comment

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

why not make it like partitionColumns = table.getPartCols() ?

Copy link
Copy Markdown
Contributor Author

@ramitg254 ramitg254 Mar 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

please check #6337 (comment)

}
if (CollectionUtils.isNotEmpty(partitionColumns) &&
conf.getBoolVar(ConfVars.HIVE_DISPLAY_PARTITION_COLUMNS_SEPARATELY)) {
Expand Down
1 change: 0 additions & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/ddl/table/partition/PartitionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.Map.Entry;

import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
Expand Down
4 changes: 1 addition & 3 deletions ql/src/java/org/apache/hadoop/hive/ql/metadata/DummyPartition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ public List<String> getValues() {
values = new ArrayList<>();

// TODO (HIVE-29413): Refactor to a generic getPartCols() implementation
for (FieldSchema fs : table.hasNonNativePartitionSupport()
? table.getStorageHandler().getPartitionKeys(table)
: table.getPartCols()) {
for (FieldSchema fs : table.getSupportedPartCols()) {
String val = partSpec.get(fs.getName());
values.add(val);
}
Expand Down
3 changes: 2 additions & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/metadata/Partition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ protected void initialize(Table table,
// set default if location is not set and this is a physical
// table partition (not a view partition)
if (table.getDataLocation() != null) {
Path partPath = new Path(table.getDataLocation(), Warehouse.makePartName(table.getPartCols(), tPartition.getValues()));
Path partPath = new Path(table.getDataLocation(),
Warehouse.makePartName(table.getPartCols(), tPartition.getValues()));
tPartition.getSd().setLocation(partPath.toString());
}
}
Expand Down
24 changes: 16 additions & 8 deletions ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,10 @@ public boolean equals(Object obj) {
&& Objects.equals(snapshotRef, other.snapshotRef);
}

public List<FieldSchema> getSupportedPartCols() {
Copy link
Copy Markdown
Member

@deniskuzZ deniskuzZ Mar 17, 2026

Choose a reason for hiding this comment

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

we should have generic API - getPartCols

Copy link
Copy Markdown
Contributor Author

@ramitg254 ramitg254 Mar 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

updating getPartCols has direct effect on implementations related to alter statements and stats autogather implementation in which some of them are limited to native ones only ( like getPartCols() should return empty list for iceberg tables even if they have keys which is required for those implementations) which will introduce more ifs at those places and even if after all that if we get a green run we can't be sure that everything would be unaffected as we can't be sure that all possible scenarios affected from it are covered by tests.

so IMHO, I think we should go for this separate api for generic use cases and older one for native use cases.

I tried to explain my thinking and one such scenario more elaboratively in this comment: #6337 (comment)

Copy link
Copy Markdown
Contributor Author

@ramitg254 ramitg254 Apr 1, 2026

Choose a reason for hiding this comment

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

@deniskuzZ can you please validate this explanation for separate api seems reasonable or are we still leaning towards having a common api with updated implementation which can lead to further if else blocks introduction on those kind of areas mentioned above

return hasNonNativePartitionSupport() ? getStorageHandler().getPartitionKeys(this) : getPartCols();
}

public List<FieldSchema> getPartCols() {
List<FieldSchema> partKeys = tTable.getPartitionKeys();
if (partKeys == null) {
Expand All @@ -610,9 +614,7 @@ public FieldSchema getPartColByName(String colName) {
}

public List<String> getPartColNames() {
List<FieldSchema> partCols = hasNonNativePartitionSupport() ?
getStorageHandler().getPartitionKeys(this) : getPartCols();
return partCols.stream().map(FieldSchema::getName)
return getSupportedPartCols().stream().map(FieldSchema::getName)
.collect(Collectors.toList());
}

Expand Down Expand Up @@ -761,10 +763,16 @@ private List<FieldSchema> getColsInternal(boolean forMs) {
* @return List&lt;FieldSchema&gt;
*/
public List<FieldSchema> getAllCols() {
ArrayList<FieldSchema> f_list = new ArrayList<FieldSchema>();
f_list.addAll(getCols());
f_list.addAll(getPartCols());
return f_list;
List<FieldSchema> allCols = new ArrayList<>(getCols());
Set<String> colNames = allCols.stream()
.map(FieldSchema::getName)
.collect(Collectors.toSet());
for (FieldSchema col : getSupportedPartCols()) {
if (!colNames.contains(col.getName())) {
allCols.add(col);
}
}
return allCols;
}

public void setPartCols(List<FieldSchema> partCols) {
Expand Down Expand Up @@ -812,7 +820,7 @@ public void setOutputFormatClass(String name) throws HiveException {
}

public boolean isPartitioned() {
return hasNonNativePartitionSupport() ? getStorageHandler().isPartitioned(this) :
return hasNonNativePartitionSupport() ? getStorageHandler().isPartitioned(this) :
CollectionUtils.isNotEmpty(getPartCols());
}

Expand Down
3 changes: 1 addition & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/optimizer/ColumnPrunerProcFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,8 +807,7 @@ public Object process(Node nd, Stack<Node> stack, NodeProcessorCtx ctx,
for (FieldNode col : cols) {
int index = originalOutputColumnNames.indexOf(col.getFieldName());
Table tab = cppCtx.getParseContext().getViewProjectToTableSchema().get(op);
List<FieldSchema> fullFieldList = new ArrayList<FieldSchema>(tab.getCols());
fullFieldList.addAll(tab.getPartCols());
List<FieldSchema> fullFieldList = new ArrayList<FieldSchema>(tab.getAllCols());
cppCtx.getParseContext().getColumnAccessInfo()
.add(tab.getCompleteName(), fullFieldList.get(index).getName());
}
Expand Down
3 changes: 2 additions & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/parse/AcidExportSemanticAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ private void analyzeAcidExport(ASTNode ast, Table exportTable, ASTNode tokRefOrN
//now generate insert statement
//insert into newTableName select * from ts <where partition spec>
StringBuilder rewrittenQueryStr = generateExportQuery(
newTable.getPartCols(), tokRefOrNameExportTable, (ASTNode) tokRefOrNameExportTable.parent, newTableName);
newTable.getPartCols(),
tokRefOrNameExportTable, (ASTNode) tokRefOrNameExportTable.parent, newTableName);
ReparseResult rr = ParseUtils.parseRewrittenQuery(ctx, rewrittenQueryStr);
Context rewrittenCtx = rr.rewrittenCtx;
rewrittenCtx.setIsUpdateDeleteMerge(false); //it's set in parseRewrittenQuery()
Expand Down
1 change: 0 additions & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsAutoGatherContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.metastore.api.FieldSchema;
Expand Down
3 changes: 1 addition & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/parse/ColumnStatsSemanticAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,7 @@ private static CharSequence genPartitionClause(Table tbl, List<TransformSpec> pa


private static String getColTypeOf(Table tbl, String partKey) {
for (FieldSchema fs : tbl.hasNonNativePartitionSupport() ?
tbl.getStorageHandler().getPartitionKeys(tbl) : tbl.getPartitionKeys()) {
for (FieldSchema fs : tbl.getSupportedPartCols()) {
if (partKey.equalsIgnoreCase(fs.getName())) {
return fs.getType().toLowerCase();
}
Expand Down
2 changes: 0 additions & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/parse/ImportSemanticAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.apache.hadoop.hive.metastore.api.StorageDescriptor;
import org.apache.hadoop.hive.metastore.ReplChangeManager;
import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;
import org.apache.hadoop.hive.ql.Context;
import org.apache.hadoop.hive.ql.ErrorMsg;
import org.apache.hadoop.hive.metastore.txn.TxnUtils;
import org.apache.hadoop.hive.ql.QueryState;
Expand Down Expand Up @@ -89,7 +88,6 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;


/**
Expand Down
3 changes: 1 addition & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.apache.hadoop.hive.ql.metadata.Table;
import org.apache.hadoop.hive.ql.parse.rewrite.MergeStatement;
import org.apache.hadoop.hive.ql.parse.rewrite.RewriterFactory;
import org.apache.hadoop.hive.ql.plan.HiveOperation;

import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -230,7 +229,7 @@
String deleteExtraPredicate) throws SemanticException {
assert whenMatchedUpdateClause.getType() == HiveParser.TOK_MATCHED;
assert getWhenClauseOperation(whenMatchedUpdateClause).getType() == HiveParser.TOK_UPDATE;
Map<String, String> newValuesMap = new HashMap<>(targetTable.getCols().size() + targetTable.getPartCols().size());
Map<String, String> newValuesMap = new HashMap<>(targetTable.getAllCols().size());

Check warning on line 232 in ql/src/java/org/apache/hadoop/hive/ql/parse/MergeSemanticAnalyzer.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace this call to the constructor with the better suited static method HashMap.newHashMap(int numMappings) 


See more on https://sonarcloud.io/project/issues?id=apache_hive&issues=AZ0aLk_qqViT6RaT494h&open=AZ0aLk_qqViT6RaT494h&pullRequest=6337
ASTNode setClause = (ASTNode)getWhenClauseOperation(whenMatchedUpdateClause).getChild(0);
//columns being updated -> update expressions; "setRCols" (last param) is null because we use actual expressions
//before re-parsing, i.e. they are known to SemanticAnalyzer logic
Expand Down
3 changes: 1 addition & 2 deletions ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,8 +581,7 @@ public static Map<Integer, List<ExprNodeGenericFuncDesc>> getFullPartitionSpecs(
CommonTree ast, Table table, Configuration conf, boolean canGroupExprs) throws SemanticException {
String defaultPartitionName = HiveConf.getVar(conf, HiveConf.ConfVars.DEFAULT_PARTITION_NAME);
Map<String, String> colTypes = new HashMap<>();
List<FieldSchema> partitionKeys = table.hasNonNativePartitionSupport() ?
table.getStorageHandler().getPartitionKeys(table) : table.getPartitionKeys();
List<FieldSchema> partitionKeys = table.getSupportedPartCols();
Copy link
Copy Markdown
Member

@deniskuzZ deniskuzZ Mar 17, 2026

Choose a reason for hiding this comment

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

prev used api is table.getPartitionKeys()

Copy link
Copy Markdown
Contributor Author

@ramitg254 ramitg254 Mar 23, 2026

Choose a reason for hiding this comment

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

yes but it should be better to use the current one as table.getPartitionKeys() throws null when no keys are there which is different from table.getStorageHandler().getPartitionKeys(table) used here which gives empty arraylist for no keys.
so with current usage it will return empty list in both scenarios and rule out null pointer exception case

for (FieldSchema fs : partitionKeys) {
colTypes.put(fs.getName().toLowerCase(), fs.getType());
}
Expand Down
2 changes: 1 addition & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/CopyOnWriteMergeRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public void appendWhenMatchedUpdateClause(MergeStatement.UpdateClause updateClau
sqlGenerator.append(hintStr);
hintStr = null;
}
List<String> values = new ArrayList<>(targetTable.getCols().size() + targetTable.getPartCols().size());
List<String> values = new ArrayList<>(targetTable.getAllCols().size());
values.addAll(sqlGenerator.getDeleteValues(Context.Operation.MERGE));
addValues(targetTable, targetAlias, updateClause.getNewValuesMap(), values);
addValuesForRowLineageForCopyOnMerge(isRowLineageSupported, values,
Expand Down
2 changes: 1 addition & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/MergeRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public void appendWhenMatchedUpdateClause(MergeStatement.UpdateClause updateClau

sqlGenerator.append(" -- update clause").append("\n");
List<String> valuesAndAcidSortKeys = new ArrayList<>(
targetTable.getCols().size() + targetTable.getPartCols().size() + 1);
targetTable.getAllCols().size() + 1);
valuesAndAcidSortKeys.addAll(sqlGenerator.getSortKeys(Operation.MERGE));
addValues(targetTable, targetAlias, updateClause.getNewValuesMap(), valuesAndAcidSortKeys);
sqlGenerator.appendInsertBranch(hintStr, valuesAndAcidSortKeys);
Expand Down
2 changes: 1 addition & 1 deletion ql/src/java/org/apache/hadoop/hive/ql/parse/rewrite/SplitMergeRewriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public void appendWhenMatchedUpdateClause(MergeStatement.UpdateClause updateClau
String onClauseAsString = mergeStatement.getOnClauseAsText();

sqlGenerator.append(" -- update clause (insert part)\n");
List<String> values = new ArrayList<>(targetTable.getCols().size() + targetTable.getPartCols().size());
List<String> values = new ArrayList<>(targetTable.getAllCols().size());
addValues(targetTable, targetAlias, updateClause.getNewValuesMap(), values);
addRowLineageColumnsForWhenMatchedUpdateClause(isRowLineageSupported, values, targetAlias, conf);
sqlGenerator.appendInsertBranch(hintStr, values);
Expand Down
Loading