[GLUTEN-6101][VL] Enable map_from_arrays function - #12976
Conversation
| } | ||
|
|
||
| test("map_from_arrays honors a lower-case mapKeyDedupPolicy value") { | ||
| withSQLConf(SQLConf.MAP_KEY_DEDUP_POLICY.key -> "last_win") { |
There was a problem hiding this comment.
Spark's SQLConf parses enum configs case-insensitively, so Gluten always receives a normalized enum value from Spark. Testing lower-case inputs in Gluten isn't necessary since that behavior is already guaranteed by Spark.
There was a problem hiding this comment.
@philo-he I see thanks for sharing that. I remove this test.
| "array(l_partkey, l_suppkey, l_linenumber))) as k from lineitem limit 10") { | ||
| checkGlutenPlan[ProjectExecTransformer] | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we consolidate the above two tests into one test with two cases included? One is for EXCEPTION policy, producing exception in duplicate case, and the other is for LAST_WIN policy.
There was a problem hiding this comment.
@philo-he Thanks for the review. Please see the latest changes. Three tests:
- Duplicate + EXCEPTION policy --> exception
- Duplicate + LAST_WIN policy --> no exception
- No Duplicate + EXCEPTION policy --> no exception
5fb5b78 to
936b59a
Compare
|
@pedrumj2, please check the test failures which should be related. Thanks. |
936b59a to
d0b8755
Compare
|
Run Gluten Clickhouse CI on x86 |
d0b8755 to
5f9e0e8
Compare
|
Run Gluten Clickhouse CI on x86 |
5f9e0e8 to
8ecddbd
Compare
|
Run Gluten Clickhouse CI on x86 |
8ecddbd to
cc29f2c
Compare
|
Run Gluten Clickhouse CI on x86 |
cc29f2c to
0576291
Compare
|
Run Gluten Clickhouse CI on x86 |
0576291 to
d1f7bfa
Compare
|
Run Gluten Clickhouse CI on x86 |
d1f7bfa to
c7bc188
Compare
|
Run Gluten Clickhouse CI on x86 |
c7bc188 to
a50df50
Compare
|
Run Gluten Clickhouse CI on x86 |
a50df50 to
ef415da
Compare
|
Run Gluten Clickhouse CI on x86 |
## What changes are proposed in this pull request? Velox gained a Spark-compatible `map_from_arrays` in [facebookincubator/velox#18630](facebookincubator/velox#18630). [apache#12968](apache#12968) pulled [a6b9f7754](facebookincubator/velox@a6b9f7754) into the Velox revision this repo pins, so the function can run natively here now. This PR removes it from [kBlackList](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc#L59). [apache#2440](apache#2440) added the denylist entry and [apache#6690](apache#6690) moved it into [kBlackList](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc#L59). [getNativeSessionConf](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala#L555) already forwards [spark.sql.mapKeyDedupPolicy](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-substrait/src/main/scala/org/apache/gluten/config/GlutenConfig.scala#L575) to Velox, and [ExpressionMappings](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionMappings.scala#L268) already maps the expression. [docs/velox-backend-scalar-function-support.md](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/docs/velox-backend-scalar-function-support.md) marks `map_from_arrays` as supported and bumps the fully-supported count from 246 to 247. Offloading the function moves who raises the error for a bad map, so several Spark UTs written while it fell back need adjusting. - [MiscOperatorSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/backends-velox/src/test/scala/org/apache/gluten/execution/MiscOperatorSuite.scala) used `map_from_arrays` as its example of an expression the backend cannot handle, so [ColumnarPartialProjectExec](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/backends-velox/src/main/scala/org/apache/gluten/execution/ColumnarPartialProjectExec.scala#L141) no longer appears in its plan. - That suite now uses [sequence](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-substrait/src/main/scala/org/apache/gluten/expression/ExpressionMappings.scala#L228), which is still in [kBlackList](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc#L59), so the validator rejects it and `ColumnarPartialProjectExec` extracts it. The query shape matters twice over. Its arguments come from `c_custkey` because a `sequence` of literals is constant-folded before the validator runs. And it selects `c_name` alongside, because [ColumnarPartialProjectExec](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/backends-velox/src/main/scala/org/apache/gluten/execution/ColumnarPartialProjectExec.scala#L136) refuses to partially project when the extracted expressions need every column the child emits; with `c_custkey` as the only referenced column, pruning makes that set identical and no partial project is built. - Spark builds the char/varchar write-side length check on a map column out of `MapFromArrays`, so that check now runs in Velox. - Velox reports a different message for it. Spark's: ``` Exceeds char/varchar type length limitation: 5 ``` and Velox's: ``` Exceeds allowed length limitation: 5 ``` - The three map overrides already in [GlutenFileSourceCharVarcharTestSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenCharVarcharTestSuite.scala#L22) switch to the Velox message, matching how the array-nested siblings in that same suite already assert. - [GlutenDSV2CharVarcharTestSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenCharVarcharTestSuite.scala#L188) gains overrides for its three map length checks and its two `SPARK-42611` map cases. - [GlutenDataFrameFunctionsSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenDataFrameFunctionsSuite.scala#L21) gains an override of `map with arrays` in all four Spark versions. - Velox raises its own error for a null map key, so that override asserts Velox's `Cannot use null as map key!` reason rather than Spark's `NULL_MAP_KEY` error class, and the cause type it checks becomes `GlutenException`. No other assertion in the test changes. - [GlutenRuntimeNullChecksV2Writes](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenRuntimeNullChecksV2Writes.scala#L19) gains overrides of three of its four map tests in Spark 3.5, 4.0 and 4.1; `NOT NULL checks for fields inside nullable maps (byName)` still passes unchanged. Spark rewrites a map cast on the write path as `map_from_arrays(transform(map_keys(m), ...), transform(map_values(m), ...))`, so the `assert_not_null` inside it now runs in Velox's [AssertNotNullFunction](https://github.com/facebookincubator/velox/blob/a6b9f7754a7ba31a0fa26c7e52d51460a6b5d450/velox/functions/sparksql/AssertNotNull.cpp#L24). That raises the same `Null value appeared in non-nullable field` reason Spark uses, but as a `VeloxUserError` surfaced as a `SparkException` caused by a `GlutenException`. The overrides keep every step of the upstream test and relax only how the error is located, walking the cause chain. Two upstream assertions do not survive that, because Velox reports neither: on Spark 3.5 the column-path match against `colPath.mkString("\n", "\n", "\n")`, and on Spark 4.x the `NOT_NULL_ASSERT_VIOLATION` condition, which is that version's only assertion. `colPath` is reported in the failure message instead. - [VeloxTestSettings](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala) excludes each upstream test that an override replaces, in all four Spark versions. Every exclusion has a matching override, so no test is dropped. - [ClickHouseTestSettings](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/clickhouse/ClickHouseTestSettings.scala#L46) excludes every Gluten override this PR adds or retargets, in all four Spark versions. The ClickHouse backend falls back to vanilla Spark for these writes, so it raises Spark's message. Only the `gluten-ut/spark34` and `gluten-ut/spark35` changes trigger the ClickHouse job, per [clickhouse_be_trigger.yml](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/.github/workflows/clickhouse_be_trigger.yml#L37-L38). Every line in an override that differs from the code it replaces carries a `// Before:` comment above it quoting that code. For an override this PR adds, that is the Spark original; for one it retargets, it is the previous Gluten assertion. Fixes apache#6101 ## How was this patch tested? ### 🟢 Step 1: map_from_arrays runs in Velox end to end This script builds the Velox backend and the Spark 3.5 jars in the CI dev image and runs the query in a real Spark session. Save it as `verify-map-from-arrays.sh` and run it against a checkout of this branch. It needs a container runtime and leaves its build tree inside the container. ```bash #!/usr/bin/env bash # Verifies end to end that map_from_arrays is offloaded to Velox. # # ./verify-map-from-arrays.sh [path-to-gluten-checkout] # # Env: # DOCKER container runtime (default: docker) # NUM_THREADS build parallelism (default: nproc) # EXTRA_DOCKER_ARGS extra flags for your runtime, e.g. proxy or network settings set -euo pipefail GLUTEN_DIR=$(cd "${1:-$PWD}" && pwd) IMAGE=apache/gluten:centos-9-jdk8 DOCKER=${DOCKER:-docker} THREADS=${NUM_THREADS:-$(nproc)} "$DOCKER" pull "$IMAGE" # shellcheck disable=SC2086 "$DOCKER" run --rm ${EXTRA_DOCKER_ARGS:-} \ -v "$GLUTEN_DIR:/work/gluten" -w /work/gluten \ -e http_proxy -e https_proxy -e no_proxy \ -e NUM_THREADS="$THREADS" \ "$IMAGE" bash -eo pipefail -c ' ./dev/buildbundle-veloxbe.sh --run_setup_script=OFF --build_arrow=OFF --spark_version=3.5 JAR=$(ls /work/gluten/package/target/gluten-velox-bundle-spark3.5_*.jar) SPARK_HOME=/opt/shims/spark35/spark_home # range() keeps the arguments non-literal so Spark cannot constant-fold the call # away before the validator sees it. cat > /tmp/q.sql <<"SQL" CREATE OR REPLACE TEMPORARY VIEW t AS SELECT id AS k, CAST(id AS STRING) AS v FROM range(5); EXPLAIN SELECT map_from_arrays(array(k, k + 1), array(v, concat(v, "x"))) AS m FROM t; SELECT map_from_arrays(array(k, k + 1), array(v, concat(v, "x"))) AS m FROM t; SQL "$SPARK_HOME"/bin/spark-sql --master "local[2]" \ --conf spark.plugins=org.apache.gluten.GlutenPlugin \ --conf spark.driver.extraClassPath="$JAR" \ --conf spark.executor.extraClassPath="$JAR" \ --conf spark.memory.offHeap.enabled=true \ --conf spark.memory.offHeap.size=2g \ --conf spark.shuffle.manager=org.apache.spark.shuffle.sort.ColumnarShuffleManager \ -f /tmp/q.sql 2>&1 | tee /tmp/verify.out sed -n "/== Physical Plan ==/,/^$/p" /tmp/verify.out > /tmp/plan.out grep -q "ProjectExecTransformer \[map_from_arrays" /tmp/plan.out ! grep -qE "^\*?\([0-9]+\) Project \[map_from_arrays" /tmp/plan.out grep -q "{0:\"0\",1:\"0x\"}" /tmp/verify.out ' echo "PASS: map_from_arrays executed in Velox as a ProjectExecTransformer" ``` The complete `/tmp/plan.out` it wrote: ``` == Physical Plan == VeloxColumnarToRow +- ^(1) ProjectExecTransformer [map_from_arrays(array(k#11L, (k#11L + 1)), array(v#12, concat(v#12, x))) AS m#3] +- ^(1) ProjectExecTransformer [id#13L AS k#11L, cast(id#13L as string) AS v#12] +- ^(1) InputIteratorTransformer[id#13L] +- ArrowColumnarToVeloxColumnar +- OffloadArrowData +- ColumnarRange 0, 5, 1, 2, 5, [id#13L] ``` The complete set of rows the `SELECT` returned: ``` {0:"0",1:"0x"} {1:"1",2:"1x"} {2:"2",3:"2x"} {3:"3",4:"3x"} {4:"4",5:"4x"} ``` And the script's own verdict: ``` PASS: map_from_arrays executed in Velox as a ProjectExecTransformer ``` This step exercises the `kBlackList` change only. It compiles none of the test files below. --- ### 🟢 Step 2: Scala formatting ```bash ./dev/format-scala-code.sh check ``` Exit 0. The script runs `build/mvn -q spotless:check` across every profile, including `spark-3.4`, `spark-3.5`, `spark-4.0`, `spark-4.1`, `spark-ut` and `backends-velox`. --- ### 🟢 Step 3: C++ formatting ```bash python3 dev/check.py format main ``` ``` Ok : cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc ``` Exit 0. Every other changed file is reported `Skip` because the checker covers `cpp,cc,h,inc,prolog` only. --- ### 🟢 Step 4: every touched module test-compiles at this revision All five modules that this PR touches compile their test sources against the Spark version they target, so every override is type-checked: ```bash build/mvn -o test-compile -Pspark-3.4 -Pjava-17 -Pbackends-velox -Pspark-ut -pl gluten-ut/spark34 -am build/mvn -o test-compile -Pspark-3.5 -Pjava-17 -Pbackends-velox -Pspark-ut -pl gluten-ut/spark35 -am build/mvn -o test-compile -Pspark-4.0 -Pscala-2.13 -Pjava-17 -Pbackends-velox -Pspark-ut -pl gluten-ut/spark40 -am build/mvn -o test-compile -Pspark-4.1 -Pscala-2.13 -Pjava-17 -Pbackends-velox -Pspark-ut -pl gluten-ut/spark41 -am build/mvn -o test-compile -Pspark-3.5 -Pjava-17 -Pbackends-velox -pl backends-velox -am ``` All five exit 0. `gluten-ut/spark41` needs Maven >= 3.8.1 for `scala-maven-plugin:4.9.5`, so it was run with Maven 3.9.9 supplied directly rather than through the wrapper, which takes whatever `mvn` is on PATH; the version this repo declares in `<maven.version>` is 3.9.16. That module's `GlutenRuntimeNullChecksV2Writes` override uses `TableInfo`, which exists only in Spark 4.1, so this is the check that confirms it resolves. ###⚠️ Step 5: suite execution — covered by CI only Running the suites needs `libgluten.so` and `libvelox.so` over JNI plus a Spark distribution at `spark.test.home`, which Step 1's container build produces but this checkout does not carry. The suites CI must cover: - [ScalarFunctionsValidateSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/backends-velox/src/test/scala/org/apache/gluten/functions/ScalarFunctionsValidateSuite.scala) — one added test, covering the offload and duplicate map keys under both `spark.sql.mapKeyDedupPolicy` values. - [MiscOperatorSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/backends-velox/src/test/scala/org/apache/gluten/execution/MiscOperatorSuite.scala) - [GlutenDataFrameFunctionsSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenDataFrameFunctionsSuite.scala), for all four Spark versions - [GlutenFileSourceCharVarcharTestSuite and GlutenDSV2CharVarcharTestSuite](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenCharVarcharTestSuite.scala), for Spark 3.4 and 3.5 - [GlutenRuntimeNullChecksV2Writes](https://github.com/apache/gluten/blob/b77fdef08e4a733d1ed424bbf807b2904b92af86/gluten-ut/spark35/src/test/scala/org/apache/spark/sql/GlutenRuntimeNullChecksV2Writes.scala), for Spark 3.5, 4.0 and 4.1 - the ClickHouse UT job, which the `gluten-ut/spark34` and `gluten-ut/spark35` changes trigger ## Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude claude-opus-5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ef415da to
324d4ec
Compare
|
Run Gluten Clickhouse CI on x86 |
What changes are proposed in this pull request?
facebookincubator/velox#18630 implemented the Spark version of
map_from_arraysin Velox.#12968 pulled that commit (a6b9f7754) into the Velox revision this repo pins.
This PR drops
map_from_arraysfromkBlackList, so a query using it now runs in Velox instead of falling back to the JVM. The function was denylisted by #2440 and moved intokBlackListby #6690.GlutenConfig.getNativeSessionConfalready forwardsspark.sql.mapKeyDedupPolicyto Velox andExpressionMappingsalready maps the expression, so nothing else had to be wired up. The scalar function support doc row is updated to mark the function supported.Fixes #6101
How was this patch tested?
Local Testing
Save this as
verify-map-from-arrays.shand run it against a checkout of this branch. It builds the Velox backend and the Spark 3.5 jars in the CI dev image, runs the query in a real Spark session, and asserts on the executed plan. It exits non-zero if the projection falls back to the JVM.Output on this branch, from a clean tree. The plan and the rows are contiguous runs from the script's own log;
[...]marks where Spark's other output was cut.The projection carrying the function is a
ProjectExecTransformer, so it ran in Velox. Restoring thekBlackListentry turns that line into*(1) Project [map_from_arrays(...)], which both of the script's plan assertions reject.Automated Tests
Four tests in
ScalarFunctionsValidateSuitecover the offload, duplicate keys under bothspark.sql.mapKeyDedupPolicyvalues, and the lower-case form of that config.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Co-authored with claude