Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-49498][SQL] Added test to check difference between SR_AI and SR_Latn_AI collations #47958

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ilicmarkodb
Copy link
Contributor

What changes were proposed in this pull request?

Added test to check difference between SR_AI and SR_Latn_AI collations.

Why are the changes needed?

Better testing.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Test added to CollationSuite.scala

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Sep 2, 2024
@@ -33,7 +35,7 @@ import org.apache.spark.sql.execution.adaptive.AdaptiveSparkPlanHelper
import org.apache.spark.sql.execution.aggregate.{HashAggregateExec, ObjectHashAggregateExec}
import org.apache.spark.sql.execution.columnar.InMemoryTableScanExec
import org.apache.spark.sql.execution.joins._
import org.apache.spark.sql.internal.{SqlApiConf, SQLConf}
import org.apache.spark.sql.internal.{SQLConf, SqlApiConf}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unneeded change

// scalastyle:off nonascii
// SR_AI collation
var collationName = "SR_AI"
var builder = new ULocale.Builder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still test spark's and not directly the ICU behavior, even though we use ICU under the hood

builder.setUnicodeLocaleKeyword("kc", "true")
collator = Collator.getInstance(builder.build())

assert(collator.compare("c", "ć") != 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably put this inside a loop to avoid repetitive calls

@HyukjinKwon
Copy link
Member

Mind filing a JIRA please? See also https://spark.apache.org/contributing.html

@ilicmarkodb ilicmarkodb changed the title Added test to check difference between SR_AI and SR_Latn_AI collations [SPARK-49498][SQL] Added test to check difference between SR_AI and SR_Latn_AI collations Sep 3, 2024
@ilicmarkodb ilicmarkodb force-pushed the add_test_for_difference_between_SR_AI_and_SR_Latn_AI_collations branch from cffc0a3 to ab0c6a5 Compare September 3, 2024 10:32
@ilicmarkodb
Copy link
Contributor Author

@cloud-fan can you please review?

("Z", "Ž")
).foreach {
case (c1, c2) =>
checkAnswer(sql(s"SELECT '$c1' = '$c2' COLLATE SR_Latn_AI"), Row(false))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also check the result for sr_ai here? It's more readable than comparing multiple characters at a time like you did above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants