HyukjinKwon commented on code in PR #59231:
URL: https://github.com/apache/spark/pull/59231#discussion_r4220204083
##########
sql/core/pom.xml:
##########
@@ -315,6 +315,19 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>
+ <dependency>
Review Comment:
**Nit (P3):** This adds sql/core test dependencies on
`spark-udf-worker-grpc` and its test-jar, and `PlanExternalUDFsSuite` now runs
`EchoGrpcWorkerMain` and the gRPC dispatcher end to end.
`dev/sparktestsupport/modules.py` isn't updated, though: `udf_worker` has no
dependents and only runs `udf-worker-core/test`, and `sql` depends only on
`catalyst`. A later PR that touches only `udf/worker/` (the echo worker, the
dispatcher, or the UDS transport) won't select the sql module, so a break in
this end-to-end path would only surface after merge. Adding `udf_worker` to the
`sql` module's dependencies would close the gap. The missing `core` ->
`udf_worker` edge predates this PR.
**Verification:**
- **Regression:** Changed-module selection for a change touching only a
udf/worker/grpc file includes the sql module.
- **Compatibility:** Selection for changes outside udf/worker/ is unchanged.
##########
sql/core/pom.xml:
##########
@@ -390,6 +403,92 @@
</execution>
</executions>
</plugin>
+ <!--
+ snowflake-jdbc 4.4.0 shades Woodstox and registers it as a StAX
factory.
+ ScalaTest's forked JVM uses Maven's test classpath, so XMLInputFactory
+ then parses sql XML tests with Woodstox. Nested arrays come back with
+ _corrupt_record, and numeric inference changes. Point the test artifact
+ at a jar with those factory registrations removed. Shaded Woodstox
+ classes stay for Snowflake.
+ -->
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>copy-snowflake-jdbc-for-stax-strip</id>
+ <phase>generate-test-resources</phase>
+ <goals>
+ <goal>copy</goal>
+ </goals>
+ <configuration>
+ <artifactItems>
+ <artifactItem>
+ <groupId>net.snowflake</groupId>
+ <artifactId>snowflake-jdbc</artifactId>
+ <version>${snowflake.jdbc.version}</version>
+
<outputDirectory>${project.build.directory}/test-classpath</outputDirectory>
+ <destFileName>snowflake-jdbc-original.jar</destFileName>
Review Comment:
**Nit (P3):** The copy target (`snowflake-jdbc-original.jar`), the Ant zip
output, and the Groovy swap path all use version-independent names. With their
documented defaults, `dependency:copy` overwrites an existing release artifact
only when the source is newer, and `<zip>` (update=false) rebuilds only when an
input is newer than the archive. Suppose you build with one
`snowflake.jdbc.version` and then, without `mvn clean`, switch to a branch
whose jar in `~/.m2` isn't newer. Both steps are skipped, and the Groovy script
silently points `net.snowflake:snowflake-jdbc` at the other version's stripped
jar. CI's clean builds aren't affected, but local runs can test against the
wrong driver. Keying these names by `${snowflake.jdbc.version}`, as the sbt
helper already does through the source jar name, would avoid it.
**Verification:**
- **Regression:** Building sql/core test resources with two different
snowflake.jdbc.version values in sequence, without clean, yields a
test-classpath Snowflake jar for the second version.
- **Inspection:** The copy output, zip source and destination, and the
artifact swap all reference the same version-keyed file names.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/externalUDF/PlanExternalUDFsSuite.scala:
##########
@@ -716,25 +719,47 @@ class PlanExternalUDFsSuite extends QueryTest with
SharedSparkSession {
assert(execution.workerSpec == spec)
}
- test("scalar external UDF execution reaches dispatcher creation") {
- val spec = workerSpec("unimplemented-scalar")
- val range = Range(0, 10, 1, 1)
- val function = udf("unimplemented-scalar", spec, Seq(singleOutput(range)))
- val resultAttr = AttributeReference("externalUDF", IntegerType, nullable =
true)()
+ test("sql tests expose one unshaded epoll native library") {
+ val resource = "META-INF/native/libnetty_transport_native_epoll_x86_64.so"
+ val urls = new java.util.ArrayList[java.net.URL]()
+ val found = getClass.getClassLoader.getResources(resource)
+ while (found.hasMoreElements) {
+ urls.add(found.nextElement())
+ }
+ assert(urls.size == 1, urls.toString)
+ assert(urls.get(0).toString.contains("netty-transport-native-epoll"))
+ }
+
+ test("scalar external UDF returns rows from a direct gRPC worker process") {
Review Comment:
**Nit (P3):** This test always spawns the echo worker over a Unix domain
socket but doesn't check that a Netty native UDS transport is available.
`DirectGrpcDispatcherIntegrationSuite` does check, with an `assume` on
`UnixDomainSocketTransport.detect()` in `beforeEach`. `udf-worker-grpc` only
ships epoll for linux-x86_64/aarch_64 and kqueue for macOS, so on hosts like
Linux ppc64le/s390x or Windows `detect()` throws and this sql/core suite fails
where the gRPC suite is canceled. Could you add the same `assume` here? Since
`UnixDomainSocketTransport` is `private[grpc]`, you could probe
`Epoll.isAvailable || KQueue.isAvailable` directly or expose a small helper
next to `EchoGrpcWorkerMain` in the grpc test-jar.
**Verification:**
- **Behavior:** On a supported host the end-to-end test still runs and
asserts the exact identity rows.
- **Regression:** When native UDS availability is reported as false, the
test is canceled rather than failed, matching the gRPC module's integration
suite.
--
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]
