andygrove commented on issue #16: URL: https://github.com/apache/datafusion-spark/issues/16#issuecomment-6855011083
I checked which functions in this crate need Spark-version or legacy-setting behavior today. To find them, I took the `spark.sql.legacy.*` and `spark.sql.function.*` settings that Spark 4.1.1's expressions and type coercion read, matched them against the functions here, and went through the open issues. ### Inventory | Function | What decides it | Status here | Issue | |---|---|---|---| | `size` | `spark.sql.legacy.sizeOfNull` and ANSI mode | Wrong whenever ANSI mode is on (returns -1, Spark returns NULL) | #90 | | `elt` | `spark.sql.function.eltOutputAsString` | Wrong at the default (returns a string for all-binary input) | #91 | | `encode` | `spark.sql.legacy.javaCharsets`, `spark.sql.legacy.codingErrorAction` (added in 4.0.0) | Implements 3.5 behavior | #46 | | `hash`, `xxhash64` | `spark.sql.legacy.allowHashOnMapType` | Accepts maps, which Spark rejects by default. Comet relies on map hashing. | #69 | | `next_day` | No setting: the error class changed between 3.5 and 4.x | Version check only | #25 | | `array_insert` | `spark.sql.legacy.negativeIndexInArrayInsert` (added in 3.4.2) | Constructor flag in the PR | #70 | | `map_from_arrays`, `map_from_entries`, `str_to_map` | `spark.sql.mapKeyDedupPolicy` | Handled, through core's `datafusion.spark.map_key_dedup_policy` | | | `format_string`, `concat`, `array()` | Legacy settings whose defaults already match | Default correct | #94 | | `CAST` | `is_spark4_plus`, `decimal_zero_any_exponent`, `decimal_skip_integral_digit_limit` in Comet | Being ported | #88 | ANSI mode is the most common of these, and its default depends on the version too: off in 3.x, on in 4.x. ### Proposed design Expressions take named behavior flags, never a version. A single module derives each flag's default from a full `major.minor.patch` Spark version. Where Spark has a setting for the behavior, the flag mirrors it, and `SET` can override it. Details are in #93. Spark backports behavior changes to patch releases, so a version enum is not precise enough. `negativeIndexInArrayInsert` arrived in 3.4.2, and Comet's [`SparkFixes`](https://github.com/apache/datafusion-comet/blob/main/spark/src/main/scala/org/apache/comet/serde/SparkFixes.scala) gates SPARK-60118 on 4.0.5, 4.1.4 and 4.2.1. Comet already works this way: its cast receives one flag per behavior, and the JVM decides each value from the running version or the real `SQLConf`. Two practical constraints shape where the flags go: - **Return types and type checks never see the config.** `return_field_from_args`, `coerce_types` and aggregate accumulators don't receive it. About half of the cases above are decided there: the nullability of `size`, the return type of `elt`, `concat` and `array()`, and the map check in `hash`. Those flags have to be set when the function is constructed and refreshed in `with_updated_config`. - **Comet doesn't pass a session config.** It builds every `ScalarFunctionExpr` with `ConfigOptions::default()`, so a flag that is only read from the config never reaches Comet. Each flag also needs a direct way in, through a constructor argument or a public function. This is the same split as `EvalMode` in #89, which has `spark_parse_url(args, EvalMode)` and `EvalMode::from_config`. The settings themselves need `SparkOptions` to move into this crate as a config extension (#92, step 1 of apache/datafusion#27342). -- 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]
