Repository navigation
[VL] Fix and enable native UDF cluster suite - #13530
kevinwilfong wants to merge 1 commit into
Conversation
b918733 to
f555bd6
Compare
philo-he
left a comment
There was a problem hiding this comment.
Thanks for the fix!
Nit: the commented-out "Run UDF test" step in velox_backend_arm.yml still uses the old single command, which would now run both suites in one JVM if re-enabled. Could we directly remove it in this PR?
| -DargLine="-Dspark.test.home=/opt/shims/spark35/spark_home/" | ||
| $MVN_CMD install -Pspark-3.5 -Pbackends-velox -Pjava-17 -Piceberg -Pdelta -Ppaimon -pl backends-velox -am -DskipTests | ||
| for suite in VeloxUdfSuiteLocal VeloxUdfSuiteCluster; do | ||
| $MVN_CMD test -pl backends-velox -Pspark-3.5 -Pbackends-velox -Pjava-17 -Piceberg -Pdelta -Ppaimon -DtagsToExclude=org.apache.gluten.tags.EnhancedFeaturesTest \ |
There was a problem hiding this comment.
Are -Piceberg -Pdelta -Ppaimon needed here? They only add lake dependencies to backends-velox, which VeloxUdfSuite doesn't use. If we drop them, drop them from the install and both test commands.
f555bd6 to
f72939c
Compare
philo-he
left a comment
There was a problem hiding this comment.
Looks good. I will merge it if there is no other review comment from the community. Thanks!
|
Thanks for the review! |
| .set("spark.driver.extraClassPath", glutenJar) | ||
| .set("spark.executor.extraClassPath", glutenJar) | ||
| .set("spark.executor.extraClassPath", sys.props("java.class.path")) | ||
| .set(VeloxConfig.NATIVE_UDF_BYPASS_REGISTRATION.key, "true") |
There was a problem hiding this comment.
@kevinwilfong Sorry for the delayed review. Is it possible to test the non-bypass configuration as well?
Run cluster executors with the test JVM classpath instead of the shaded package jar. This matches the already-running unshaded driver classes and fixes UDAF serialization failures. Remove gluten.package.jar and the driver classpath setting that cannot change the running JVM. Build absolute driver UDF library URIs so relative cpp.build.dir works. Select the cluster suite with UDFTest and run each suite in a separate JVM. Install required modules once before selecting suites in backends-velox. Set SPARK_SCALA_VERSION=2.12 so executor launch does not require Spark source build directories. Trigger UDF CI for suite, workflow and native changes. Remove unused lake profiles and the obsolete commented-out ARM UDF step. Run both suites with bypass registration disabled and enabled. Retain Hive UDF/UDAF coverage in both modes, verify native-only names are unavailable with bypass disabled, and run the by-name tests with bypass enabled. Remove the forced bypass configuration from the suite implementations. Validated locally November 10 against base d64a51d with matching rebuilt native libraries, Spark3.5.5, Scala2.12 and JDK17. Both suites pass 5 tests with bypass disabled and 8 with it enabled. Each invocation also passes 20 JUnit tests. No tests skipped. Formatter, license and workflow checks passed. Updated Linux GitHub Actions execution is pending. Originally authored with Claude Code; updated and validated with Codex. Generated-by: Codex CLI 0.164.0
f72939c to
26510af
Compare
|
Test failure is in GlutenTransformWithStateAvroSuite which I believe is flaky |
The failure didn't occur after a rerun. |
| checkAnswer(df, Seq(Row(2L), Row(3L), Row(4L))) | ||
| } | ||
| if (bypassRegistration) { | ||
| test("native udf with a plain name is callable without a hive udf class") { |
There was a problem hiding this comment.
It would be better to use a helper function testBypassRegistration for these tests rather than put them into a if.
| } | ||
| } else { | ||
| test("native udf names are not registered when bypass registration is disabled") { | ||
| val queries = Seq( |
There was a problem hiding this comment.
And add if (!bypassRegistration) only in this test.
What changes are proposed in this pull request?
The VeloxUdfSuiteCluster test is currently broken in trunk and not enabled in CI.
Fix and enable VeloxUdfSuiteCluster in the dedicated UDF CI job.
How was this patch tested?
Validated locally with rebuilt native libraries, Spark 3.5.5, Scala 2.12, and JDK 17: both suites passed all eight tests, with none skipped.
Verified the CI run on this PR.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: GPT-6.1 Sol