Skip to content

test: cover a native Iceberg read of a data file that omits an optional nested field - #7359

Open
yunchipang wants to merge 4 commits into
apache:mainfrom
yunchipang:test/iceberg-omitted-optional-nested-field
Open

yunchipang wants to merge 4 commits into
apache:mainfrom
yunchipang:test/iceberg-omitted-optional-nested-field

Conversation

@yunchipang

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #7033.

Rationale for this change

The native Iceberg scan failed with Incorrect number of arrays for StructArray fields, expected 2 got 1 when a data file omitted an optional nested field without a schema change. The iceberg-rust bump in #7028 fixed this, since iceberg-rust now matches nested fields by field id and fills in missing ones with NULLs. Nothing tested this case.

What changes are included in this PR?

A test in CometIcebergNativeSuite that writes a data file without s.b and l.element.y, using write.spark.accept-any-schema. It then checks that the native scan matches Spark, with nested schema pruning on and off.

How are these changes tested?

The new test passes on main. On the parent of #7028, it fails with the error from the issue.

@github-actions github-actions Bot added enhancement New feature or request test Testing related area:Iceberg labels Oct 31, 2026

@sunchao sunchao left a comment

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.

The new regression passed on Spark 4.1.3 with Iceberg 1.11. I inspected the Parquet footers produced by the same fixture and confirmed that the narrow files omit both nested leaves. One assertion would preserve that coverage as the writer changes.

SELECT 3, CAST(NULL AS STRUCT<a: INT>), CAST(NULL AS ARRAY<STRUCT<x: INT>>)
""")
.writeTo(table)
.append()

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.

Could the test check that at least one appended Parquet file lacks both s.b and the list element’s y leaf? If a writer upgrade materializes them as all-null columns, both Spark comparisons and the native-plan assertion will still pass, and the test will stop exercising #7033. This suite already reads file_path from the Iceberg files table and opens its footer with ParquetFileReader.

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.

thanks! added a footer check.

@sunchao sunchao left a comment

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.

Reviewed 801755b with five independent scopes. The new assertions first verify the nested leaf paths against a full-schema file and then require a physical file omitting both leaves, so they preserve the intended coverage as the writer changes. I compiled the exact updated suite and ran this regression on Spark 4.1.3 / Iceberg 1.11; it passed with the native scan assertions. The production sources are unchanged from the previously tested revision. No remaining actionable issue found.

@manuzhang manuzhang added the run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue label Nov 4, 2026
@manuzhang

Copy link
Copy Markdown
Member

@yunchipang Please remove these lines from the doc.

- **A data file that omits an optional nested field.** The native scan fails on a data file that
lacks an optional struct field, which a write with `write.spark.accept-any-schema=true` can
produce without any schema change
([#7033](https://github.com/apache/datafusion-comet/issues/7033)).

@manuzhang

Copy link
Copy Markdown
Member

@yunchipang Meanwhile, could you please enable Github Actions on your fork repository such that CI can run there?

@yunchipang
yunchipang force-pushed the test/iceberg-omitted-optional-nested-field branch from f0dea03 to df8300b Compare November 4, 2026 18:52

@comphead comphead left a comment

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.

Thanks @yunchipang, the footer check makes this a solid regression test for #7033.

Comment on lines +2279 to +2289
val reader = org.apache.parquet.hadoop.ParquetFileReader.open(
org.apache.parquet.hadoop.util.HadoopInputFile.fromPath(
new org.apache.hadoop.fs.Path(row.getString(0)),
spark.sessionState.newHadoopConf()))
try {
reader.getFooter.getFileMetaData.getSchema.getColumns.asScala
.map(_.getPath.mkString("."))
.toSet
} finally {
reader.close()
}

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.

This is the third copy in this suite of opening a footer with ParquetFileReader.open(HadoopInputFile.fromPath(new Path(...), spark.sessionState.newHadoopConf())) and closing it in a finally, after the page-index test around line 1024 and the INT96 test around line 4653. Each uses the reader differently, so a small private withParquetReader[T](path: String)(f: ParquetFileReader => T): T would cover all three and keep the fully qualified names in one place. Would you add it here?

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.

thanks for the review! added the share reader at line 169

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

Labels

area:Iceberg enhancement New feature or request run-all-spark-profiles Run the Comet test suites against every Spark profile on this pull request, ahead of the merge queue test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native Iceberg scan fails when a data file omits an optional nested field

4 participants