Use table schema from the table handle

[findinpath]

Description

In the context of the dealing with an Iceberg table with a structure which evolves over time (columns are added / dropped) in case of performing a snapshot/time travel query, the schema of the output matches the corresponding schema of the table snapshot queried.

Fixes #14064
Relates to #12786

Non-technical explanation

In the context of time travel queries, use the table schema corresponding to the snapshot of the table queried
for retrieving the columns of the output.

Release notes

( ) This is not user-visible 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
* Use table schema corresponding to snapshot in snapshot queries

[alexjo2144]

Looks like there are some other places we do loadTable().getSchema() like this:

• getTableProperties - still needs to load the table, but should use the schema from the handle
• getInsertLayout - this one doesn't practically matter because you can only insert on the latest snapshot, but should still change it
• TableStatisticsMaker#makeTableStatistics - also shouldn't matter, but might want to change it

I think only the first one really matters

[findinpath]

@alexjo2144 this is actually a point that I wanted to bring in this PR - IcebergMetadata at the time of this writing has 15 places where of table.schema(). Would it be safe to reuse everywhere the schema passed through the IcebergTableHandle ?

[alexjo2144]

Yeah I think we should prefer the schema in the Handle. The pattern I was looking for here were methods which called loadTable().getSchema() and had an IcebergTableHandle as input. If the method takes a SchemaTableName, loading the table and using that schema is fine.

[findepi]

• getInsertLayout - this one doesn't practically matter because you can only insert on the latest snapshot, but should still change it

let's make sure cleanups like this and the bug fix come in separate commits

[findinpath]

let's make sure cleanups like this and the bug fix come in separate commits

I created a separate PR to avoid cluttering the current changes

#14079

[findepi]

Due to internal caching within the method
org.apache.iceberg.ManifestGroup.planFiles
the returned file scan tasks may contain an invalid split
schema string.

is it testable?

[findinpath]

Yes, it it testable through io.trino.plugin.iceberg.TestIcebergAvroConnectorTest.

I was reluctant on squashing the two commits of this PR because they address different issues.

The test io.trino.plugin.iceberg.TestIcebergAvroConnectorTest covers both of the issues.

[findepi]

this is a good change.

However, it looks like we call SchemaParser.fromJson(tableHandle.getTableSchemaJson()) multiple times on one table handle. Am i right?

SchemaParser.fromJson does cache internally (on a static field).
This isn't ideal, and we could better, caching within table handle object. Not sure it matters though -- depends how frequently this is called.

[findinpath]

Should we switch to SchemaParser.fromJson(JsonNode)

fromJson(JsonUtil.mapper().readValue(jsonKey, JsonNode.class))

?