HIVE-29413: Generalise column related APIs in Table.java by ramitg254 · Pull Request #6413 · apache/hive

ramitg254 · 2026-04-07T12:51:34Z

What changes were proposed in this pull request?

added getEffectivePartCols() in most places possible to avoid code duplication.

Why are the changes needed?

getPartCols() does not have support for iceberg tables.

Does this PR introduce any user-facing change?

No

How was this patch tested?

ci tests and local build

deniskuzZ · 2026-04-09T20:12:19Z

@ramitg254 please take a look: 9e7535c. I would suggest following similar approach

ramitg254 · 2026-04-10T05:36:07Z

9e7535c

but here we are creating separate method getEffectivePartCols() and leaving getPartCols() as it is, which as per our discussion on that closed pr we shouldn't do that, and only go ahead with updating getPartCols()

deniskuzZ · 2026-04-10T06:50:49Z

9e7535c

but here we are creating separate method getEffectivePartCols() and leaving getPartCols() as it is, which as per our discussion on that closed pr we shouldn't do that, and only go ahead with updating getPartCols()

Where did I say that? The ask was to keep the original method unchanged. same here

ramitg254 · 2026-04-10T07:08:55Z

oh I got confused due to this comment: #6337 (comment) in which getSupportedPartCols() was just separate method similar to getEffectivePartCols()

ramitg254 · 2026-04-10T07:18:52Z

I am fine with that earlier approach as well but recently I saw this one: https://issues.apache.org/jira/browse/HIVE-29525 so I thought we should have unified getPartCols() and getCols() which gives similar results as native hive tables as first step towards solving this after that those plan logics can be taken care of later on when that ticket will be addressed.
So I was first focussing on making getPartCols() unified for iceberg tables as well.

please share your thoughts on this idea

# Conflicts: # iceberg/iceberg-handler/src/test/results/positive/llap/iceberg_bucket_map_join_7.q.out

Change-Id: I09ffba356ac47e3416c8b6717e8671d2cb6432b8

Change-Id: I6812d068be702edf01b158c3e7eacadeadbd5c2b

Change-Id: I6f5d81c5be00e080753efd5e8157b14fcfde2dde

Change-Id: I5a95791b4d756fe949ee4697204be9de3025c6b5

Change-Id: I852cf156a1a5ac20525bb9376f4cbb5783b8e1e3

Change-Id: Ib1542e38c65c7811074b97d7da7aaf780f3895dd

Change-Id: Ie3440239248870fb44c6e956d5be10271b28a1a0

Change-Id: Ic8f6d6c391ba49e12de7d04b7bb20542d879b0a3

Change-Id: I2017f7c771f8f611b5d8900d3e0276817842b41f

Change-Id: I87dc65eb942e3db1b1a0d9f2608f1b88b44b11e6

Change-Id: Ic0fefede8b079205c5580a31f59f195b6b1e94e0

Change-Id: I5a34151d5ec0d4e1759eee5fcc6339b5fd6b92d3

Change-Id: I0b130777712dd05df53a1ca6ed80ea4c5abd0187

Change-Id: Id1ce8250addc158a36887d4c8538ee2ab18b6f39

Change-Id: Ic5ec07bc8c9c1c656407fcaec08904792dd61093

deniskuzZ · 2026-06-24T13:21:05Z

   */
  public void setTTable(org.apache.hadoop.hive.metastore.api.Table tTable) {
    this.tTable = tTable;
+    tablePartCols = null;


consider extracting it to a private self-describing reset method (i.e. resetColumnCache etc)

tablePartCols = null; tableNonPartCols = null; columnsByName = null;

is done in at least 3 places

Change-Id: I518f8baac535b65fad0295bd1c892457c904d8e9

sonarqubecloud · 2026-06-25T06:59:20Z

Quality Gate passed

Issues
21 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

kasakrisz · 2026-06-25T11:54:55Z

+        sqlGenerator.append(identifier);
+        // This is one of the columns we're setting, record it's position so we can come back
+        // later and patch it up. 0th is ROW_ID
+        setColExprs.put(setColExprIndex + columnOffset, setCol);


Simplify: in this method only the result of this expressionsetColExprIndex + columnOffset is used but two parameters are passed. Please pass the result of the expression and remove one of the parameters.

kasakrisz · 2026-06-25T11:58:38Z

+    if (prependComma) {
+      sqlGenerator.append(",");
+    }


Please move this back to the loop where this method is called.
We always append non-partition columns and in the next step we add partition columns. In case of non-partition columns the the comma is not needed but in case of every follow-up columns it is. And also partition columns need it but when we add partition columns we continue appending to an existing list so all columns need a comma. As a result please remove prependComma parameter of this method and the appendPartitionColumns

kasakrisz · 2026-06-25T12:04:01Z

+    List<FieldSchema> cols = table.hasNonNativePartitionSupport() ?
+        table.getAllCols() : table.getCols();


What is blocking collecting the lineage information of all columns in case of native tables?

kasakrisz · 2026-06-25T12:13:49Z

+      boolean isStaticPartition = partSpec != null && partSpec.containsKey(partColName)
+          && partSpec.get(partColName) != null;


Isn't partSpec.containsKey(partColName) redundant here?

kasakrisz · 2026-06-25T12:42:43Z

+      // Partition cols come from the handler, but user comments are stored in HMS sd.getCols().
+      // Copy HMS comments onto handler fields so DESCRIBE/SHOW match native table behavior.


Please add that these can be invalid in the case of imported non-native tables, because the storage handler might not support user comments.

asf-ci-hive added tests pending tests unstable and removed tests pending labels Apr 7, 2026

ramitg254 force-pushed the HIVE-29413 branch from 0d2baee to d97e174 Compare April 8, 2026 13:31

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

ramitg254 force-pushed the HIVE-29413 branch from d97e174 to 9e87b12 Compare April 8, 2026 18:21

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

ramitg254 force-pushed the HIVE-29413 branch from 9e87b12 to 565a2eb Compare April 9, 2026 10:03

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

ramitg254 mentioned this pull request Apr 9, 2026

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

Closed

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

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

asf-ci-hive added tests pending and removed tests unstable labels Apr 10, 2026

ramitg254 added 24 commits June 22, 2026 19:21

updated ctas create and tests output

a4a9ce6

# Conflicts: # iceberg/iceberg-handler/src/test/results/positive/llap/iceberg_bucket_map_join_7.q.out

updated stats autogather and test output

c58da98

updated getPartitionKeys

4ddced3

removed getStorageSchemaCols part-1

19b47ee

removed getStorageSchemaCols part-2

565e1f6

removed getStorageSchemaCols part-3

dd870bb

removed workaround

350541d

addressed sonar issues

1de739d

non part cols retrieval made lazy

664b31f

reviewed required changes

001f82e

Change-Id: I09ffba356ac47e3416c8b6717e8671d2cb6432b8

corrected partition.getCols for iceberg table

b4f8f3a

Change-Id: I6812d068be702edf01b158c3e7eacadeadbd5c2b

added wrapper for lineage

5f3d8cd

Change-Id: I6f5d81c5be00e080753efd5e8157b14fcfde2dde

reverted getPartitionKeys

b42b0dc

Change-Id: I5a95791b4d756fe949ee4697204be9de3025c6b5

refractor-1

57d16b7

Change-Id: I852cf156a1a5ac20525bb9376f4cbb5783b8e1e3

refractor-2

261bb88

Change-Id: Ib1542e38c65c7811074b97d7da7aaf780f3895dd

correction for rebased iceberg view commit recently merged to master

850275e

Change-Id: Ie3440239248870fb44c6e956d5be10271b28a1a0

removed isTableTypeSet and merged partition column comments

f19f7af

Change-Id: Ic8f6d6c391ba49e12de7d04b7bb20542d879b0a3

updated conflicts for rebase

ebbc002

Change-Id: I2017f7c771f8f611b5d8900d3e0276817842b41f

reverted to user comment override and refractored

cb23165

Change-Id: I87dc65eb942e3db1b1a0d9f2608f1b88b44b11e6

moved helpers to MetaStoreUtils

3829bcb

Change-Id: Ic0fefede8b079205c5580a31f59f195b6b1e94e0

moved to HiveTableUtil

2d4ab11

Change-Id: I5a34151d5ec0d4e1759eee5fcc6339b5fd6b92d3

updated method name

095807e

Change-Id: I0b130777712dd05df53a1ca6ed80ea4c5abd0187

refractor

0cf7727

Change-Id: Id1ce8250addc158a36887d4c8538ee2ab18b6f39

refractor-2

f1fbeba

Change-Id: Ic5ec07bc8c9c1c656407fcaec08904792dd61093

deniskuzZ reviewed Jun 24, 2026

View reviewed changes

refractored Table metadata and merge rewriter

150eb0a

Change-Id: I518f8baac535b65fad0295bd1c892457c904d8e9

kasakrisz reviewed Jun 25, 2026

View reviewed changes

		List<FieldSchema> cols = table.hasNonNativePartitionSupport() ?
		table.getAllCols() : table.getCols();

		boolean isStaticPartition = partSpec != null && partSpec.containsKey(partColName)
		&& partSpec.get(partColName) != null;

		// Partition cols come from the handler, but user comments are stored in HMS sd.getCols().
		// Copy HMS comments onto handler fields so DESCRIBE/SHOW match native table behavior.

Uh oh!

Conversation

ramitg254 commented Apr 7, 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 commented Apr 9, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

deniskuzZ Jun 24, 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!

ramitg254 Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Jun 25, 2026

Quality Gate passed

Uh oh!

kasakrisz Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

kasakrisz Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

kasakrisz Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

kasakrisz Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

kasakrisz Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ramitg254 commented Apr 7, 2026 •

edited

Loading

ramitg254 commented Apr 10, 2026 •

edited

Loading

deniskuzZ commented Apr 10, 2026 •

edited

Loading

ramitg254 commented Apr 10, 2026 •

edited

Loading

ramitg254 commented Apr 10, 2026 •

edited

Loading

deniskuzZ Jun 24, 2026 •

edited

Loading