This happens in some cases, I found it in some case of importing external files to Iceberg table, ie TestIcebergSourceHadoopTables.testFilesTableWithSnapshotIdInheritance, where the I think columns are out of order of the original schema and the metrics are corrupt (underflow exception in this case).

Not sure if we should error out the files tables, in that case, I was leaning towards just returning null. User has original column to see why the error happened.

Do you have examples when this throws an exception?

Yea I tried a put a comment, but unfortunately got disassociated with this line after a rebase.

This happens in some cases, I found it in some case of importing external files to Iceberg table, ie TestIcebergSourceHadoopTables.testFilesTableWithSnapshotIdInheritance, where the I think columns are out of order of the original schema and the metrics are corrupt (underflow exception in this case).

Not sure if we should error out the files tables, in that case, I was leaning towards just returning null. User has original column to see why the error happened.

@aokolnychyi filed an issue: #5543

Following up on this, this is a non-issue as the spark procedures set the flag: schema.name-mapping.default , just this test does not. Fixed the test.

What about simply adapting the existing join method? Are there any scenarios where we want to skip the validation and simply add all columns (old logic)?

I think this logic should be part of BaseFilesTableScan and BaseAllFilesTableScan.
Otherwise, our scans won't report the correct schema in Scan$schema().

I think putting it there will break the scan right, as its not the projection the user requested.

Note, this is actually a bit subtle here. Because we are doing the join, (original projection + minimum metrics), the file's schema becomes
{any_projected_field_on_file} : {readable_metrics because its also projected} : {un-projected but required metrics fields}

So the ContentFileWithMetrics works because it will discard any of the "un-projected but required metrics fields", given they are outside the range it will read. For the remaining fields it uses the existing logic (delegate to file for the first n-1, and then get from MetricsStruct for nth field).

I mean, we could add a select method to GenericDataFile to modify its internal 'fromProjectionPos' map to conform back to the original projection (dropping the "un-projected but required metrics fields"). But it would mainly be for clarity, and not strictly needed.

nit: This changes the original behavior, why not add a new function?

@chenjunjiedada Yea that was my original version, and changed after comment of @aokolnychyi #5376 (comment)

Technically this is changing a public API which previously would have allowed these combos. This is ... maybe ok since it's a utility method but we may end up breaking users of the function at runtime. That said I think Anton is right and any schema with multiple columns with the same ID would always be wrong.

Test used to write the wrong metrics to imported table, without these lines.

I don't believe this has to be atomic, and metadataTableSchema should already have a method highestFieldId() which according to the doc includes nested fields

If you prefer incrementAndGet() to ++nextId though I think using the Atomic just for readability is probably fine

Ah AtomicInteger is because there is a lambda function in there (the map) and compiler complains :

Variable used in lambda expression should be final or effectively final

Re: highestFieldId(), good to know! Done.

Just to make this a little easier in the future you may just want to do something like

firstAssigned = (schema.highestId - 15)
Then do
1001 = firstAssigned +1; ....

not sure this really helps that much though

Done