Skip to content

[BUILD] Give iceberg-test its own build-helper execution ids - #12946

Open
LuciferYang wants to merge 4 commits into
apache:mainfrom
LuciferYang:iceberg-profile-exec-id
Open

[BUILD] Give iceberg-test its own build-helper execution ids#12946
LuciferYang wants to merge 4 commits into
apache:mainfrom
LuciferYang:iceberg-profile-exec-id

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

The iceberg and iceberg-test profiles each declared a build-helper-maven-plugin execution named add-iceberg-test-sources, and another named add-iceberg-test-resources. Maven merges plugin executions by id, so activating both profiles kept only one <sources> list per id. This gives the iceberg-test pair its own ids and changes nothing else.

Fixes #12933.

Before the change, help:effective-pom with -Pbackends-clickhouse -Pspark-3.5 -Pspark-ut -Pdelta -Piceberg -Piceberg-test resolved add-iceberg-test-sources to src-iceberg-spark35/test/{scala,java} alone. The iceberg profile's src-iceberg/test/{scala,java} and src-iceberg${iceberg.binary.version}/test/{scala,java} were dropped with no warning. After the change all four executions survive in both backends.

For ClickHouse the old behaviour made that profile combination unbuildable. ClickHouseIcebergHiveTableSupport lives in src-iceberg/test/scala and the Java test under src-iceberg-spark35/test/java instantiates it, so the class left the source path at exactly the moment the test joined it:

$ ./build/mvn -Pbackends-clickhouse -Pspark-3.5 -Pscala-2.13 -Pspark-ut -Pdelta \
      -Piceberg -Piceberg-test -DskipTests test-compile -pl backends-clickhouse -am
[ERROR] .../src-iceberg-spark35/test/java/.../TestPositionDeletesTableGluten.java:[41,18] cannot find symbol
[ERROR]   symbol:   class ClickHouseIcebergHiveTableSupport

Velox was unaffected because nothing there needed both directories at once: the src-iceberg/test suites run in the jobs that pass -Piceberg alone, and the files under src-iceberg-spark34/test run in the three groups that pass both.

The one behaviour change a reviewer should weigh is that those three groups, velox_backend_x86.yml:817, :821 and :825, plus the Spark 4.0 test-compile at :1277, now also compile src-iceberg/test/**. VeloxIcebergSuite and VeloxTPCHIcebergSuite therefore run twice across the matrix, once there and once in the -Piceberg-only jobs at :941 and :1013. That costs job time and gains nothing by itself, so if it is unwelcome the alternative is to drop -Piceberg from those three commands, which I did not do here because it would change which dependencies they resolve as well.

src-iceberg${iceberg.binary.version}/test and every src-iceberg*/test/resources directory are empty in all modules today, so the resources rename and that half of the sources rename are latent fixes with no effect on the current tree. Only backends-velox and backends-clickhouse hold files under src-iceberg*/test, so nothing outside those two modules is touched.

While checking for the same shape elsewhere in the root pom, the only other cross-profile id reuse is enforce-java-17+ and enforce-scala-213, shared by the spark-4.0 and spark-4.1 profiles. Those two are mutually exclusive, so the merge never happens and I left them alone.

How was this patch tested?

All four runs are on e0dce586b with only this change applied, JDK 17.

what result
ClickHouse test-compile, -Piceberg -Piceberg-test, before the change fails on ClickHouseIcebergHiveTableSupport
the same command after the change exit 0, emits ClickHouseIcebergHiveTableSupport.class and TestPositionDeletesTableGluten.class
Velox test-compile with the profile set the three x86 groups use (-Pjava-17 -Pspark-3.4 -Pspark-ut -Piceberg -Piceberg-test -Pdelta -Phudi -Ppaimon) BUILD SUCCESS
help:effective-pom for both backends after the change all four executions present, six test source directories each

The Velox run is the one that matters for regressions, since it is the combination that newly compiles five more files. It emitted VeloxIcebergSuite, VeloxTPCHIcebergSuite, enhanced/VeloxIcebergSuite and TestIcebergNestedFieldVisitor while keeping the existing src-iceberg-spark34/test classes.

I did not run the ClickHouse tests themselves. TestPositionDeletesTableGluten compiles for the first time here but no job passes -Piceberg-test to a ClickHouse build, so it still does not execute; #12934 covers whether it should.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude claude-opus-5

Copilot AI lite review requested due to automatic review settings September 1, 2026 08:47

Copilot AI 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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Resolves Maven build-helper-maven-plugin execution-id collisions between the iceberg and iceberg-test profiles so both profiles can be activated together without losing test source/resource paths.

Changes:

  • Renames build-helper-maven-plugin execution ids to avoid Maven merging executions across profiles.
  • Preserves separate add-test-source and add-test-resource executions so all intended test directories remain on the effective POM/classpath.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pom.xml
<executions>
<execution>
<id>add-iceberg-test-sources</id>
<id>add-iceberg-spark-test-sources</id>
Comment thread pom.xml
</execution>
<execution>
<id>add-iceberg-test-resources</id>
<id>add-iceberg-spark-test-resources</id>
@github-actions github-actions Bot added the CORE works for Gluten Core label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Copilot AI review requested due to automatic review settings September 4, 2026 03:13
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Copilot AI 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.

🟢 Approval recommended

The change is narrowly scoped to Maven execution IDs, resolves a verified effective-POM merge bug, and does not alter the actual source/resource directory lists.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@LuciferYang

Copy link
Copy Markdown
Contributor Author

Good prompt, though I landed somewhere slightly different.

I kept the ids naming what the execution adds (src-iceberg-spark${spark.plain.version}/test/...) rather than which profile owns it, because the profile-based scheme is what created the confusion in the first place: the iceberg profile's own test execution is already called add-iceberg-test-sources, and that is the name that reads as if it belonged to iceberg-test. Renaming by owner would mean renaming that one too, which is four more executions in a profile the collision does not otherwise touch.

What the comment is really after, though, is that the reason for the difference should be visible at the point where someone might undo it. So I said it out loud instead of encoding it in the id:

<!-- These ids must stay different from the iceberg profile's
     add-iceberg-test-sources / add-iceberg-test-resources: Maven merges plugin
     executions by id, so sharing one would drop the other profile's sources
     whenever both profiles are active, which is how every in-repo build runs. -->

That also covers the future-collision half of your point better than a naming convention would, since nothing enforces a convention.

Also merged current main, which moved the two colliding executions from pom.xml:1861 and pom.xml:1912 to :1833 and :1884 after #12902 dropped the spark-3.3 profile. #12933's line numbers are updated (the description never carried them). Re-ran the check that matters on the new base: test-compile -Pbackends-clickhouse -Pspark-3.5 -Pspark-ut -Pdelta -Piceberg -Piceberg-test exits 0 and emits both ClickHouseIcebergHiveTableSupport.class and TestPositionDeletesTableGluten.class, which is the pair that cannot compile without this fix.

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

Labels

CORE works for Gluten Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUILD] iceberg and iceberg-test profiles collide on one build-helper execution id, silently dropping src-iceberg/test sources

2 participants