minor: refactor Spark ascii function to reuse DataFusion ascii function code #17965
Conversation
| test_ascii!(Some(String::from("\n")), Ok(Some(10))); | ||
| test_ascii!(Some(String::from("\t")), Ok(Some(9))); |
There was a problem hiding this comment.
Consolidating the tests here, removing from Spark unit test (Spark slt still remains)
| /// [ascii]: https://spark.apache.org/docs/latest/api/sql/index.html#ascii | ||
| /// [default ascii function]: datafusion_functions::string::ascii::AsciiFunc | ||
| #[derive(Debug, PartialEq, Eq, Hash)] | ||
| pub struct SparkAscii { |
There was a problem hiding this comment.
I considered removing this entirely and making AsciiFunc have a toggleable behaviour for this, but keeping it like this was easier to work with considering existing macros used to export the udf's
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @Jefffrey please clarify if ascii can support numeric like in
https://spark.apache.org/docs/latest/api/sql/#ascii
> SELECT ascii(2);
50
Sorry, do you mean in the DF version function doc or the Spark version function doc? |
Slightly confused. Can |
Yes it can; it seems it casts the |
No problem, I only realized myself when the existing Spark ascii SLT test caught it when I tried to make them exactly the same functions: datafusion/datafusion/sqllogictest/test_files/spark/string/ascii.slt Lines 38 to 41 in f8ff82a |
2 participants
Which issue does this PR close?
Part of #17964
Rationale for this change
Some of the internals are the same; reuse this code.
What changes are included in this PR?
Refactor SparkAscii to reuse the DataFusion Ascii internal code, adding doc comments to SparkAscii to explain difference with DataFusion version (hence why they are separate functions). Also update some of the Spark function documentation to clarify behaviour when it has overlapping function names with DataFusion functions.
Are these changes tested?
Existing tests
Are there any user-facing changes?
No