HIVE-29413: Avoid code duplication by updating getPartCols method for iceberg tables by ramitg254 · Pull Request #6337 · apache/hive

ramitg254 · 2026-02-24T18:46:02Z

What changes were proposed in this pull request?

updated getPartCols method and introduced a boolean parameter in it to handle partition keys needed for non native tables and to get rid of this kind of code redundancy in some places:

table.hasNonNativePartitionSupport() ?
            table.getStorageHandler().getPartitionKeys(table) :
            table.getPartCols();

Why are the changes needed?

to avoid code redundancy and have a common method to retrieve keys for both native and non native tables.

Does this PR introduce any user-facing change?

No

How was this patch tested?

locally

deniskuzZ · 2026-03-17T15:55:48Z

hcatalog/core/src/test/java/org/apache/hive/hcatalog/mapreduce/TestHCatMultiOutputFormat.java

    org.apache.hadoop.hive.ql.metadata.Table tbl = hive.getTable(database, table);
    FetchWork work;
-    if (!tbl.getPartCols().isEmpty()) {
+    if (!tbl.getSupportedPartCols().isEmpty()) {


please keep the old method signature

deniskuzZ · 2026-03-17T15:58:00Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/column/show/ShowColumnsOperation.java

-    allColumns.addAll(table.getCols());
-    allColumns.addAll(table.getPartCols());
-    return allColumns;
+    return new ArrayList<>(table.getAllCols());


why do we need to clone it?

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());

deniskuzZ · 2026-03-17T15:59:51Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/create/like/CreateTableLikeOperation.java


    table.setFields(oldTable.getCols());
-    table.setPartCols(oldTable.getPartCols());
+    table.setPartCols(oldTable.getNativePartCols());


this is super confusing!!!

table.setPartCols <- oldTable.getNativePartCols

kept the old signature

deniskuzZ · 2026-03-17T16:01:06Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/info/desc/formatter/TextDescTableFormatter.java

-        partitionColumns = table.hasNonNativePartitionSupport() ?
-            table.getStorageHandler().getPartitionKeys(table) :
-            table.getPartCols();
+        partitionColumns = table.getSupportedPartCols();


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

please check #6337 (comment)

deniskuzZ · 2026-03-17T16:01:32Z

ql/src/java/org/apache/hadoop/hive/ql/ddl/table/info/desc/DescTableOperation.java

        table.getCols() : partition.getCols());
    if (!desc.isFormatted()) {
-      cols.addAll(table.getPartCols());
+      cols.addAll(table.getNativePartCols());


keep the old signature

deniskuzZ · 2026-03-17T16:04:05Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java

  }

-  public List<FieldSchema> getPartCols() {
+  public List<FieldSchema> getSupportedPartCols() {


we should have generic API - getPartCols

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)

@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

deniskuzZ · 2026-03-17T16:04:26Z

ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java

-    f_list.addAll(getCols());
-    f_list.addAll(getPartCols());
-    return f_list;
+    ArrayList<FieldSchema> allCols = new ArrayList<>(getCols());


code to Interface

deniskuzZ · 2026-03-17T16:06:54Z

ql/src/java/org/apache/hadoop/hive/ql/parse/ParseUtils.java

    Map<String, String> colTypes = new HashMap<>();
-    List<FieldSchema> partitionKeys = table.hasNonNativePartitionSupport() ?
-        table.getStorageHandler().getPartitionKeys(table) : table.getPartitionKeys();
+    List<FieldSchema> partitionKeys = table.getSupportedPartCols();


prev used api is table.getPartitionKeys()

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

deniskuzZ · 2026-03-17T16:07:37Z

ql/src/java/org/apache/hadoop/hive/ql/stats/ColStatsProcessor.java

            }
          } else {
-            partColSchema.addAll(tbl.getPartCols());
+            partColSchema.addAll(tbl.getNativePartCols());


keep the method signature unchanged

…iceberg tables

sonarqubecloud · 2026-03-23T13:32:18Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ramitg254 · 2026-04-06T09:40:12Z

closing this pull request for now as we should have single method and other places where it is getting used should be updated in such a way that it should be inclusive of updated logic

deniskuzZ · 2026-04-09T14:35:46Z

hi @ramitg254, since you put this activity on-hold, would you mind if I take it over?

ramitg254 · 2026-04-09T14:54:28Z

Hi, I am still working on it on this pr: #6413, it is in wip right now as I am resolving test failures corresponding to that.
although it's in wip, I'll appreciate your review on it

asf-ci-hive added tests pending tests unstable and removed tests pending labels Feb 24, 2026

ramitg254 force-pushed the HIVE-29413 branch from d6824f0 to cecf57e Compare March 1, 2026 07:19

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Mar 1, 2026

ramitg254 force-pushed the HIVE-29413 branch from cecf57e to fb592e2 Compare March 6, 2026 09:55

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Mar 6, 2026

ramitg254 force-pushed the HIVE-29413 branch from fb592e2 to af3436f Compare March 8, 2026 15:03

asf-ci-hive added tests pending tests unstable and removed tests unstable tests pending labels Mar 8, 2026

ramitg254 force-pushed the HIVE-29413 branch from af3436f to 44b0243 Compare March 9, 2026 18:06

asf-ci-hive added tests pending and removed tests unstable labels Mar 9, 2026

ramitg254 force-pushed the HIVE-29413 branch from 44b0243 to ea200d1 Compare March 9, 2026 18:12

asf-ci-hive added tests failed tests pending tests unstable and removed tests pending tests failed labels Mar 9, 2026

ramitg254 force-pushed the HIVE-29413 branch from ea200d1 to caaec51 Compare March 10, 2026 07:24

asf-ci-hive removed the tests unstable label Mar 10, 2026

asf-ci-hive added tests pending tests passed and removed tests pending labels Mar 10, 2026

ramitg254 requested review from abstractdog and deniskuzZ March 10, 2026 18:07

deniskuzZ reviewed Mar 17, 2026

View reviewed changes

asf-ci-hive added tests pending tests failed and removed tests passed tests pending labels Mar 23, 2026

ramitg254 force-pushed the HIVE-29413 branch 2 times, most recently from 8043508 to c6c6cdd Compare March 23, 2026 08:43

asf-ci-hive added tests pending and removed tests failed labels Mar 23, 2026

ramitg254 requested a review from deniskuzZ March 23, 2026 08:55

asf-ci-hive removed the tests pending label Mar 23, 2026

ramitg254 added 2 commits March 23, 2026 14:26

HIVE-29413:Avoid code duplication by updating getPartCols method for …

56b8cd6

…iceberg tables

updated api usage

0d0641e

Conversation

ramitg254 commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

deniskuzZ Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramitg254 Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramitg254 Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Mar 23, 2026

Quality Gate passed

Uh oh!

ramitg254 commented Apr 6, 2026

Uh oh!

deniskuzZ commented Apr 9, 2026

Uh oh!

ramitg254 commented Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ramitg254 commented Feb 24, 2026 •

edited

Loading

deniskuzZ Mar 17, 2026 •

edited

Loading

ramitg254 Mar 23, 2026 •

edited

Loading

ramitg254 Mar 23, 2026 •

edited

Loading