[AURON #1680] initCap semantics are aligned with Spark #1681
Conversation
|
@slfan1989 @richox could you please take a look? |
| ColumnarValue::Scalar(ScalarValue::Utf8(Some(str))) => { | ||
| Ok(ColumnarValue::Scalar(ScalarValue::Utf8(Some(initcap(str))))) | ||
| } | ||
| _ => df_execution_err!("string_initcap only supports literal utf8"), |
There was a problem hiding this comment.
how about "string_initcap only accepts string input"?
| ("select initcap(null)", Row(null))).foreach { case (q, expected) => | ||
| checkAnswer(sql(q), Seq(expected)) | ||
| withTable("initcap_basic_tbl") { | ||
| sql(s"CREATE TABLE initcap_basic_tbl(id INT, txt STRING) USING parquet") |
There was a problem hiding this comment.
It would better not to use parquet table to do testing, the test would fail on second run because it would create a directory on local disk, on second run, the directory name is already taken.
Use temp view or something similar would be better.
There was a problem hiding this comment.
Great suggestion. We'll fix this in a follow-up PR.
| ColumnarValue::Scalar(ScalarValue::Utf8(Some(str))) => { | ||
| Ok(ColumnarValue::Scalar(ScalarValue::Utf8(Some(initcap(str))))) | ||
| } | ||
| _ => df_execution_err!("string_initcap only supports literal utf8"), |
There was a problem hiding this comment.
yes, obviously it supports both literal and non-literal string inputs.
| sql(s""" | ||
| |INSERT INTO initcap_mixed_tbl VALUES | ||
| | (1, 'a1b2 c3D4'), | ||
| | (2, '---abc--- ABC --ABC-- 世界 世 界 '), |
There was a problem hiding this comment.
Sorry about that! I think it would be better to remove the Chinese test cases here and use English instead. This way, other team members can more easily understand and verify the code during review. Thanks for understanding!
There was a problem hiding this comment.
it is ok to use chinese (or any other non-ascii characters) because we have to test with real unicode strings.
There was a problem hiding this comment.
Thanks to @richox for the feedback. From the perspective of Unicode testing, I believe this is acceptable.
yew1eb
force-pushed
the
impl_spark_initcap
branch
from
096fbcd to
a4d206e
Compare
5 participants
Which issue does this PR close?
Closes #1680 .
Rationale for this change
The current initcap implementation uses DataFusion's initcap, which does not match Spark's semantics. Spark uses space-only word boundaries and title-cases the first letter while lowercasing the rest.
What changes are included in this PR?
string.asInstanceOf[UTF8String].toLowerCase.toTitleCase.Are there any user-facing changes?
Yes. initcap results will now match Spark's semantics.
How was this patch tested?
Added unit tests covering ASCII/non-ASCII, punctuation, space-only boundaries, and edge cases.