sunchao commented on PR #5854: URL: https://github.com/apache/datafusion-comet/pull/5854#issuecomment-5744116922
Re-reviewed `5ba49238b385b44809a3a913308244254eba06ef` against base `8c229a703ccb024a8b5b1b849a56ceef0b66a4bd`. The previous repeated-action case is fixed, and its regression passes. One P2 remains before landing. **[P2] Align policy capture with Spark's builder initialization** [`MapBuilderSupport.dedupPolicy`](https://github.com/apache/datafusion-comet/blob/5ba49238b385b44809a3a913308244254eba06ef/spark/src/main/scala/org/apache/comet/serde/maps.scala#L153) now captures the policy during physical planning. With AQE disabled, `df.explain()` converts and caches the Comet plan without initializing Spark's map builder. Changing the setting before the first action therefore gives different behavior: ```scala spark.conf.set("spark.sql.adaptive.enabled", "false") val path = java.nio.file.Files.createTempDirectory("map-policy").resolve("data").toString spark.range(0, 1, 1, 1).write.parquet(path) spark.conf.set("spark.sql.mapKeyDedupPolicy", "EXCEPTION") val df = spark.read.parquet(path) .selectExpr("map_from_arrays(array(id, id), array(1, 2)) AS m") df.explain() spark.conf.set("spark.sql.mapKeyDedupPolicy", "LAST_WIN") df.collect() ``` Spark returns `{0 -> 2}`, but Comet raises `DUPLICATED_MAP_KEY`. In the reverse direction, explaining under `LAST_WIN` and first collecting under `EXCEPTION`, Spark rejects the duplicate while Comet returns `{0 -> 2}`. This affects `map_from_arrays`, `map_from_entries`, and `str_to_map`. Could we align capture with Spark's builder initialization while keeping the policy stable on subsequent actions, and add coverage for explaining before first execution? The new repeated-Dataset test executes the query before changing the setting, so it does not cover this case. **Validation:** I reproduced all six cases end to end with the exact-head JNI library, the unmodified DataFusion 55.1.0 lockfile, Spark 4.1.3, and JDK 17. The probes cover all three constructors in both directions, assert that a Comet projection is present before execution, and verify that the explained plan is reused. All 35 existing map-suite tests and 22 native wrapper tests passed. The six diagnostic probes pass when the mismatch is reproduced; the root-reactor Maven run and its XML reports confirm all 41 selected JVM tests completed. The native build disabled the optional HDFS feature, and these probes use local Parquet. **CI:** 23 checks succeeded and 14 were skipped, including successful Required Checks. [Run 35435933319](https://github.com/apache/datafusion-comet/actions/runs/35435933319) passed 1,599 native tests, four separate allocator-accounting tests, and 1,530 Spark expression tests. CI ran merge revision `d4fdf24a1b31e61d3984b23216c1975baccd9b48`; the affected map sources and new planner helper match the reviewed head, while unrelated portions of the merge tree differ. -- 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]
