Support partitioning on nested ROW fields in Iceberg

[krvikash]

Description

Fixes #15109
Support partitioning on nested ROW fields in Iceberg

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(X) Release notes are required, with the following suggested text:

# Iceberg
* Add support for partitioning on nested ROW fields. ({issue}`15712`)

[ebyhr]

@krvikash Could you rebase on upstream to resolve conflicts?

[krvikash]

Rebased the PR with latest code.

[krvikash]

TODO task. Parquet format returning null for partition nested field.

[findinpath]

If Spark Iceberg can't handle this kind of use-case please see whether there is already reported an issue to address this missing functionality.
We shouldn't introduce TODOs in the code relating to Spark limitations. Trino does not depend on Spark.

Simply add an assertion that the query is failing with an expected message.

[krvikash]

I could not find any existing issue for PARQUET issue. Where Saprk returns null for the partitioned nested field.

[krvikash]

Hi, @alexjo2144 | @ebyhr | @findepi | @findepi, when you get time could you please review this?

[krvikash]

Rebased the PR with latest code.

[findinpath]

If Spark Iceberg can't handle this kind of use-case please see whether there is already reported an issue to address this missing functionality.
We shouldn't introduce TODOs in the code relating to Spark limitations. Trino does not depend on Spark.

Simply add an assertion that the query is failing with an expected message.

[findinpath]

Same as for Parquet.

[krvikash]

I could not find any existing issue for PARQUET issue. Where Saprk returns null for the partitioned nested field.

[alexjo2144]

Can you also add some predicate pushdown tests? Similar to one of these tests that use isFullyPushedDown https://github.com/trinodb/trino/blob/master/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java#L1611

[alexjo2144]

This block is a bit confusing to me since it relies on getParentSourceId returning the field id that was passed to it if it's already a base column. Is an integer column the parent of itself? That's fuzzy. I might rephrase it as

boolean isBaseColumn = !parentIndex.contains(fieldId);
if (isBaseColumn) {
    ...
}
else {
    ...
}

[findinpath]

This doesn't seem right to me.
Doing tricks like this can get us into trouble (there are some syntethic columns - see org.apache.iceberg.MetadataColumns#FILE_PATH ) which have the id set to Integer.MAX_VALUE - 1. Incrementing this value to get artificially a new field id for the row can lead to problems.

[findinpath]

I was looking for where schemaFromHandles is being used and found the following:

• deletes handling in IcebergPageSourceProvider
• bucketing function in IcebergNodePartitioningProvider

In both places we do have schema available.

Please investigate whether using schema in schemaFromHandles would be possible.
If yes, we could work with TypeUtil.indexParents(schema) to get only the extra "row" contents we need from the schema.

[alexjo2144]

This is related to some changes in #14837

Maybe we can pull some of those changes in here

[krvikash]

Thanks, @findinpath | @alexjo2144 for pointing this out. I have added some changes from #14837 and now I do not need to reassign indexes.

IMO, It will be better if #14837 gets merged first because the current PR contains changes, that are unrelated to the supporting partitioning field.

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[findepi]

@krvikash @alexjo2144 @findinpath I see no approvals and some yet unresolved conversations. Where are we with the PR?

[ebyhr]

I believe this PR is waiting for #14837 (#15712 (comment))

[krvikash]

Rebased on top of #14837 's commit and resolved conflicts. This PR (2nd commit of this PR) is ready for review now.

[findinpath]

It is hard to follow what is happening when we have mutations.

Why do we remove the first element from the list?

[ebyhr]

The first element is used for getting the root field id, but the element shouldn't exist for IcebergColumnHandle.path. I updated the comment and slightly modified the logic.

[ebyhr]

Rebased on master to resolve conflicts.

[findinpath]

Pls rebase the code to resolve conflicts.

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua