sam-1112 commented on PR #5802: URL: https://github.com/apache/datafusion-comet/pull/5802#issuecomment-5667547063
@andygrove Thanks for the detailed review. I addressed the comments in `0f0bcc124` and `5275b4397`, and rebased the branch onto the latest `main`. I kept the general dispatcher routing instead of adding a shape-specific allowlist or preserving the old fallback only for shuffle. The benchmark does not give us a stable rule for such an allowlist: performance varies with map size, key width, operator context, and hardware, while map size is only known at runtime. This PR is therefore about correctness and finer-grained fallback, not a throughput improvement. The documentation now makes that explicit and describes the targeted rollback: ```text spark.comet.expression.MapSort.enabled=false ``` This restores the previous whole-operator fallback for `MapSort` without disabling the dispatcher globally. I also addressed the other points: - Updated the diagnostic to say that these key types have no native implementation, instead of saying they are unsupported. - Added collated-string map-key coverage with `spark.sql.collation.allowInMapKeys=true`. - Added exact `spark_partition_id()` parity checks against Spark for array- and struct-keyed native shuffles. - Enabled `spark.sql.legacy.disableMapKeyNormalization=true` while creating the strict-FP fixtures and verified that `+0.0`, `-0.0`, and `NaN` are present. ## Refreshed benchmark I rebuilt Spark 4.1 from `5275b4397` and reran the matched benchmark in five fresh JVMs. Both arms keep the global dispatcher enabled. The only difference is `spark.comet.expression.MapSort.enabled`. - 5/5 runs completed successfully. - 80/80 correctness and routing checks passed. - Values below are median `[min, max]`. - A positive delta means the dispatcher was slower. | Shape | Workload | Fallback, ms | Dispatcher, ms | Delta | |---|---|---:|---:|---:| | `array-small` | projection | 132 `[127, 144]` | 184 `[179, 188]` | +39.4% | | `array-small` | shuffle | 166 `[160, 172]` | 232 `[231, 245]` | +39.8% | | `array-large-map` | projection | 794 `[779, 888]` | 1,447 `[1,415, 1,519]` | +82.2% | | `array-large-map` | shuffle | 738 `[724, 799]` | 1,435 `[1,392, 1,719]` | +94.4% | | `array-wide` | projection | 206 `[196, 233]` | 398 `[395, 420]` | +93.2% | | `array-wide` | shuffle | 256 `[250, 273]` | 352 `[339, 389]` | +37.5% | | `struct-small` | projection | 186 `[147, 189]` | 163 `[154, 165]` | −12.4% | | `struct-small` | shuffle | 189 `[184, 194]` | 345 `[332, 350]` | +82.5% | | `struct-large-map` | projection | 1,235 `[855, 1,250]` | 1,212 `[1,179, 1,313]` | −1.9% | | `struct-large-map` | shuffle | 928 `[896, 946]` | 2,363 `[2,326, 2,411]` | +154.6% | | `struct-wide` | projection | 354 `[237, 363]` | 500 `[492, 555]` | +41.2% | | `struct-wide` | shuffle | 365 `[352, 379]` | 807 `[791, 823]` | +121.1% | | `double-small` | projection | 121 `[116, 129]` | 138 `[130, 140]` | +14.0% | | `double-small` | shuffle | 111 `[109, 114]` | 165 `[163, 168]` | +48.6% | | `double-large-map` | projection | 522 `[506, 584]` | 1,058 `[1,021, 1,064]` | +102.7% | | `double-large-map` | shuffle | 485 `[479, 520]` | 935 `[932, 953]` | +92.8% | The dispatcher was slower in 14 of 16 cases. Projection was about 38.8% slower by geometric mean, and shuffle was about 80.0% slower, with no shuffle wins. `struct-small` projection had a better median, but the ranges overlap. `struct-large-map` projection was effectively neutral. So I am not claiming a performance benefit here. The benefit is keeping the enclosing operator in Comet while using Spark-exact `MapSort.doGenCode`, with the MapSort-specific setting available when that tradeoff is not desirable. -- 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]
