Skip to content

HIVE-29525: Iceberg partitioned columns are not reflected by RelOptHiveTable - #6600

Open
kasakrisz wants to merge 5 commits into
apache:masterfrom
kasakrisz:HIVE-29525-master-relopthivetable
Open

HIVE-29525: Iceberg partitioned columns are not reflected by RelOptHiveTable#6600
kasakrisz wants to merge 5 commits into
apache:masterfrom
kasakrisz:HIVE-29525-master-relopthivetable

Conversation

@kasakrisz

Copy link
Copy Markdown
Contributor

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?

mvn test -Dtest.output.overwrite -Dtest=TestIcebergCliDriver -Dqfile=merge_iceberg_copy_on_write_partitioned.q,merge_iceberg_copy_on_write_unpartitioned.q,merge_iceberg_orc.q,merge_iceberg_partitioned_orc.q,merge_with_null_check_on_joining_col.q,update_iceberg_copy_on_write_partitioned.q,update_iceberg_copy_on_write_unpartitioned.q,update_iceberg_partitioned_avro.q,update_iceberg_partitioned_orc.q,update_iceberg_partitioned_parquet.q,update_iceberg_unpartitioned_parquet.q -pl itests/qtest-iceberg -Pitests

TableScan [TS_0] (rows=238 width=89)
default@srcbucket_mapjoin_part_1_n1,a,Tbl:COMPLETE,Col:COMPLETE,Grouping Num Buckets:2,Grouping Partition Columns:["key"],Output:["key","part"]
TableScan [TS_0] (rows=238 width=188)
default@srcbucket_mapjoin_part_1_n1,a,Tbl:COMPLETE,Col:PARTIAL,Grouping Num Buckets:2,Grouping Partition Columns:["key"],Output:["key","part"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this (and other tests) moving from Col:COMPLETE to Col:PARTIAL because partitioned columns are now considered, and there are no stats for them? Or is it another reason?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The explanation fits my understanding. One of the partitioned columns doesn't support statistics. Maybe the bucket(2, key) in srcbucket_mapjoin_part_1_n1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In the expression bucket(2, key) currently Hive can't treat key as a partition column. In the first version of the patch I tried but this leads to incomplete stats.

File Output Operator [FS_32]
table:{"name:":"default.bucketmapjoin_tmp_result"}
Select Operator [SEL_31] (rows=785 width=366)
Select Operator [SEL_31] (rows=238000 width=366)

@deniskuzZ deniskuzZ Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like an over-estimate?
cc @okumin

TableScan [TS_3] (rows=238 width=89)
default@srcbucket_mapjoin_part_2_n4,b,Tbl:COMPLETE,Col:COMPLETE,Output:["key","part"]
TableScan [TS_3] (rows=238 width=188)
default@srcbucket_mapjoin_part_2_n4,b,Tbl:COMPLETE,Col:PARTIAL,Output:["key","part"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this expected change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same as #6600 (comment)

@@ -306,17 +306,17 @@ STAGE PLANS:
TableScan
alias: srcpart_iceberg
filterExpr: ds is not null (type: boolean)
Statistics: Num rows: 2000 Data size: 188000 Basic stats: COMPLETE Column stats: COMPLETE
Statistics: Num rows: 2000 Data size: 368000 Basic stats: COMPLETE Column stats: PARTIAL

@deniskuzZ deniskuzZ Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this expected change? why Column stats is PARTIAL?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As of version: s1
filterExpr: (a > 2) (type: boolean)
Statistics: Num rows: 1 Data size: 0 Basic stats: PARTIAL Column stats: COMPLETE
Statistics: Num rows: 1 Data size: 4 Basic stats: COMPLETE Column stats: PARTIAL

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

partitioned by spec (a), where a > 2 why it's PARTIAL ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

throw new CalciteCteException("Failed to create temporary location", e);
}
Table hiveTable = new Table(metaTable);
hiveTable.setFields(columns.stream()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems unrelated to the PR, do we have tests for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is called when a CTE is materialized at the CBO level. A temporary table is created to store the CTE result set and the table's fields were not set.

public static <T> ImmutableMap<Integer, T> getColInfoMap(List<T> hiveCols,
int startIndx) {
Builder<Integer, T> bldr = ImmutableMap.<Integer, T> builder();
public static ImmutableMap<Integer, ColumnInfo> getColInfoMap(List<ColumnInfo> hiveCols, Table table) {

@deniskuzZ deniskuzZ Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how about

public static ImmutableMap<Integer, ColumnInfo> getColInfoMap(
    List<ColumnInfo> hiveCols, ToIntFunction<String> indexByName) {
  return Maps.uniqueIndex(hiveCols, ci -> indexByName.applyAsInt(ci.getInternalName()));
}

and then

this.hiveNonPartitionColsMap =
    HiveCalciteUtil.getColInfoMap(hiveNonPartitionCols, hiveTblMetadata::getColumnIndexByName);

int allColCount = tabMetaData.getAllCols().size();
List<ColumnInfo> colInfoList = new ArrayList<>(Collections.nCopies(allColCount, null));
Set<String> partColNames = new HashSet<>(tabMetaData.getPartColNames());
ArrayList<ColumnInfo> nonPartitionColumns = new ArrayList<>(fields.size());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

code to interface

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

change to

List<ColumnInfo> nonPartitionColumns...

}

final TableType tableType = obtainTableType(tabMetaData);
List<ColumnInfo> partitionColumns = new ArrayList<>(partitionColumnSet);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

List.copyOf(partitionColumnSet)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

// 3.2 Add column info corresponding to partition columns
// Normally, the column names in a schema should be unique, but in the case of Iceberg v1 tables,
// updating the partition spec doesn't remove the existing partition keys, so we can end up with a
// partition spec containing multiple columns with the same name.

@deniskuzZ deniskuzZ Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you elaborate? shouldn't the type be void in case column is dropped as part of schema evolution?

@kasakrisz
kasakrisz force-pushed the HIVE-29525-master-relopthivetable branch from f7f65ba to e671c7b Compare August 6, 2026 13:22
@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants