[BUILD] Give iceberg-test its own build-helper execution ids - #12946
[BUILD] Give iceberg-test its own build-helper execution ids#12946LuciferYang wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
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-pluginexecution ids to avoid Maven merging executions across profiles. - Preserves separate
add-test-sourceandadd-test-resourceexecutions 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.
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟢 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
|
Good prompt, though I landed somewhere slightly different. I kept the ids naming what the execution adds ( 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 |
What changes are proposed in this pull request?
The
icebergandiceberg-testprofiles each declared abuild-helper-maven-pluginexecution namedadd-iceberg-test-sources, and another namedadd-iceberg-test-resources. Maven merges plugin executions by id, so activating both profiles kept only one<sources>list per id. This gives theiceberg-testpair its own ids and changes nothing else.Fixes #12933.
Before the change,
help:effective-pomwith-Pbackends-clickhouse -Pspark-3.5 -Pspark-ut -Pdelta -Piceberg -Piceberg-testresolvedadd-iceberg-test-sourcestosrc-iceberg-spark35/test/{scala,java}alone. Theicebergprofile'ssrc-iceberg/test/{scala,java}andsrc-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.
ClickHouseIcebergHiveTableSupportlives insrc-iceberg/test/scalaand the Java test undersrc-iceberg-spark35/test/javainstantiates it, so the class left the source path at exactly the moment the test joined it:Velox was unaffected because nothing there needed both directories at once: the
src-iceberg/testsuites run in the jobs that pass-Picebergalone, and the files undersrc-iceberg-spark34/testrun 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,:821and:825, plus the Spark 4.0test-compileat:1277, now also compilesrc-iceberg/test/**.VeloxIcebergSuiteandVeloxTPCHIcebergSuitetherefore run twice across the matrix, once there and once in the-Piceberg-only jobs at:941and:1013. That costs job time and gains nothing by itself, so if it is unwelcome the alternative is to drop-Picebergfrom those three commands, which I did not do here because it would change which dependencies they resolve as well.src-iceberg${iceberg.binary.version}/testand everysrc-iceberg*/test/resourcesdirectory 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. Onlybackends-veloxandbackends-clickhousehold files undersrc-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+andenforce-scala-213, shared by thespark-4.0andspark-4.1profiles. 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
e0dce586bwith only this change applied, JDK 17.test-compile,-Piceberg -Piceberg-test, before the changeClickHouseIcebergHiveTableSupportClickHouseIcebergHiveTableSupport.classandTestPositionDeletesTableGluten.classtest-compilewith the profile set the three x86 groups use (-Pjava-17 -Pspark-3.4 -Pspark-ut -Piceberg -Piceberg-test -Pdelta -Phudi -Ppaimon)help:effective-pomfor both backends after the changeThe 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/VeloxIcebergSuiteandTestIcebergNestedFieldVisitorwhile keeping the existingsrc-iceberg-spark34/testclasses.I did not run the ClickHouse tests themselves.
TestPositionDeletesTableGlutencompiles for the first time here but no job passes-Piceberg-testto 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