LuciferYang commented on PR #12902:
URL: https://github.com/apache/gluten/pull/12902#issuecomment-5477432114
Thanks, all three were worth catching. Two are fixed in `099bed2f0`; the
third turned out not to be a copy.
**Golden plans.** You are right, I missed them. `VeloxTPCHSuite` derives the
directory from the running Spark version (`spark.version.replace(".",
"").substring(0, 2)`), so `tpch-approved-plan/{v1,v1-bhj}/spark33` was
unreachable. Removed, 42 files. The same line of reasoning applies to
`shouldCheckGoldenFiles`, whose `case "33" => true` branch can no longer be
selected, so that goes with them.
**Velox.md.** Added, using the version the code actually requires rather
than a round number: `iceberg-core-1.10.0`'s class files are major version 55,
so Java 11 is the floor, and both `dev/release/build-release.sh` and the
Iceberg-enabled CI jobs build this combination with JDK 17. The comment now
reads `# For spark3.5.x with Iceberg (requires JDK 11+; CI and the release
build use JDK 17)`.
**TestFlinkUpsert.** This one cannot be copied as it stands, and the reason
is a version skew I had not noticed. The `spark-3.3` profile in
`backends-clickhouse/pom.xml` was empty, so that variant compiled against the
file's default `iceberg.flinkConnector.subversion=1.16`, while the `spark-3.5`
profile overrides it to `1.19`. I copied the file into `src-iceberg-spark35`
and compiled: `org.apache.iceberg.flink.MiniClusterResource` does not resolve.
Checking the jars, `iceberg-flink-1.19-1.10.0-tests.jar` ships
`CatalogTestBase`, `TestHelpers` and `MiniFlinkClusterExtension`, but no
`MiniClusterResource`; Iceberg replaced it when the Flink test base moved to
JUnit 5 extensions.
So porting it means migrating the test to the 1.19 test API, not moving a
file, and I cannot validate that locally: my `test-compile` of
`backends-clickhouse` also fails to resolve `ClickHouseIcebergHiveTableSupport`
for the pre-existing `TestPositionDeletesTableGluten`, so I have no way to tell
a real failure from my own setup, and I have no environment to run the CH Flink
tests in. I would rather not push an untested rewrite of a 539-line test
through a removal PR.
Shall I file an issue for it and reference it from this PR, or would you
prefer to take it? Worth noting either way that the Flink dependency block in
`backends-clickhouse/pom.xml` is left with no test using it once this lands, so
whoever picks it up may want to fold that in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
