[SPARK-48935][SQL][TESTS] Make `checkEvaluation` directly check the `Collation` expression itself in UT #47401

panbingkun · 2024-07-18T08:39:34Z

What changes were proposed in this pull request?

The pr aims to:

make checkEvaluation directly check the Collation expression itself in UT, rather than Collation(...).replacement.
fix an miss check in UT.

Why are the changes needed?

When checking the RuntimeReplaceable expression in UT, there is no need to write as checkEvaluation(Collation(Literal("abc")).replacement, "UTF8_BINARY"), because it has already undergone a similar replacement internally.

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

Line 75 in 1a428c1

case r: RuntimeReplaceable => replace(r.replacement)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Update existed UT.
Pass GA.

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

No.

… constructor of `StringType`

panbingkun · 2024-07-18T08:52:11Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala

@@ -28,14 +28,14 @@ class CollationExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
    assert(collationId == 0)
    val collateExpr = Collate(Literal("abc"), "UTF8_BINARY")
    assert(collateExpr.dataType === StringType(collationId))
-    collateExpr.dataType.asInstanceOf[StringType].collationId == 0


fix an miss check

panbingkun · 2024-07-18T08:53:59Z

...c/test/scala/org/apache/spark/sql/catalyst/expressions/CollationRegexpExpressionsSuite.scala

-    val nullStr = Literal.create(null, StringType)
-    // Supported collations (StringTypeBinaryLcase)
-    val binaryCollation = StringType(CollationFactory.collationNameToId("UTF8_BINARY"))
-    val lowercaseCollation = StringType(CollationFactory.collationNameToId("UTF8_LCASE"))
    // LikeAll


Although the following modifications are not related to this PR, they are only made to maintain consistency with the rest of the checks in this PR.

panbingkun · 2024-07-18T08:57:45Z

cc @cloud-fan

cloud-fan · 2024-07-18T10:56:33Z

sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala

@@ -88,7 +88,10 @@ class StringType private(val collationId: Int) extends AtomicType with Serializa
 */
 @Stable
 case object StringType extends StringType(0) {
-  private[spark] def apply(collationId: Int): StringType = new StringType(collationId)
+  private[spark] def apply(collationId: Int): StringType = {
+    assert (collationId >= 0 && collationId <= (1 << 12))


where did we check collation id before?

It did not check collationId, it only checked collationName,

spark/sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala

Line 94 in 1a428c1

val collationId = CollationFactory.collationNameToId(collation)

spark/common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java

Line 391 in 1a428c1

throw collationInvalidNameException(originalName);

spark/common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java

Line 615 in 1a428c1

throw collationInvalidNameException(originalName);

Additionally, exceptions may be thrown below due to invalid collationId, but unfortunately, they are all functions that only trigger when called

spark/sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala

Lines 32 to 46 in 83a9791

class StringType private(val collationId: Int) extends AtomicType with Serializable {

/**

* Support for Binary Equality implies that strings are considered equal only if

* they are byte for byte equal. E.g. all accent or case-insensitive collations are considered

* non-binary. If this field is true, byte level operations can be used against this datatype

* (e.g. for equality and hashing).

*/

def supportsBinaryEquality: Boolean =

CollationFactory.fetchCollation(collationId).supportsBinaryEquality

def isUTF8BinaryCollation: Boolean =

collationId == CollationFactory.UTF8_BINARY_COLLATION_ID

def isUTF8BinaryLcaseCollation: Boolean =

collationId == CollationFactory.UTF8_LCASE_COLLATION_ID

cloud-fan · 2024-07-18T10:57:41Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollationExpressionSuite.scala

@@ -67,16 +67,16 @@ class CollationExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
  }

  test("collation on non-explicit default collation") {
-    checkEvaluation(Collation(Literal("abc")).replacement, "UTF8_BINARY")
+    checkEvaluation(Collation(Literal("abc")), "UTF8_BINARY")


does checkEvaluation take care of RuntimeReplaceable?

Yea, as follows:

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

Lines 87 to 90 in 1a428c1

protected def checkEvaluation(

expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {

// Make it as method to obtain fresh expression everytime.

def expr = prepareEvaluation(expression)

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

Lines 79 to 82 in 1a428c1

private def prepareEvaluation(expression: Expression): Expression = {

val serializer = new JavaSerializer(new SparkConf()).newInstance()

val resolver = ResolveTimeZone

val expr = resolver.resolveTimeZones(replace(expression))

recursion

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala

Lines 74 to 75 in 1a428c1

protected def replace(expr: Expression): Expression = expr match {

case r: RuntimeReplaceable => replace(r.replacement)

cloud-fan · 2024-07-23T13:57:32Z

sql/api/src/main/scala/org/apache/spark/sql/types/StringType.scala

@@ -88,7 +88,10 @@ class StringType private(val collationId: Int) extends AtomicType with Serializa
 */
 @Stable
 case object StringType extends StringType(0) {
-  private[spark] def apply(collationId: Int): StringType = new StringType(collationId)
+  private[spark] def apply(collationId: Int): StringType = {


I checked several callers of this function. The input collation id is mostly calculated from collation name. This assertion doesn't seem to be necessary and it's not cheap. Shall we revert?

I'm fine with other cleanups in this PR.

Okay, I have revert it.
Thank you!

…Collation` expression itself in UT

cloud-fan · 2024-07-24T06:43:44Z

thanks, merging to master!

…Collation` expression itself in UT ### What changes were proposed in this pull request? The pr aims to: - make `checkEvaluation` directly check the `Collation` expression itself in UT, rather than `Collation(...).replacement`. - fix an `miss` check in UT. ### Why are the changes needed? When checking the `RuntimeReplaceable` expression in UT, there is no need to write as `checkEvaluation(Collation(Literal("abc")).replacement, "UTF8_BINARY")`, because it has already undergone a similar replacement internally. https://github.com/apache/spark/blob/1a428c1606645057ef94ac8a6cadbb947b9208a6/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L75 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Update existed UT. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#47401 from panbingkun/SPARK-48935. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-48935][SQL] Restrictions oncollatinId should be added to the…

d5ac399

… constructor of `StringType`

github-actions bot added the SQL label Jul 18, 2024

panbingkun commented Jul 18, 2024

View reviewed changes

update

e42c52f

panbingkun marked this pull request as ready for review July 18, 2024 08:57

cloud-fan reviewed Jul 18, 2024

View reviewed changes

fix

8223a57

cloud-fan reviewed Jul 23, 2024

View reviewed changes

panbingkun added 2 commits July 24, 2024 07:20

Merge branch 'master' into SPARK-48935

b18cd04

[SPARK-48935][SQL][TESTS] Make checkEvaluation directly check the `…

69f9a2a

…Collation` expression itself in UT

panbingkun changed the title ~~[SPARK-48935][SQL] Restrictions oncollatinId should be added to the constructor of StringType~~ [SPARK-48935][SQL] Make checkEvaluation directly check the Collation expression itself in UT Jul 23, 2024

panbingkun changed the title ~~[SPARK-48935][SQL] Make checkEvaluation directly check the Collation expression itself in UT~~ [SPARK-48935][SQL][TESTS] Make checkEvaluation directly check the Collation expression itself in UT Jul 24, 2024

cloud-fan closed this in 4de4ed1 Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-48935][SQL][TESTS] Make `checkEvaluation` directly check the `Collation` expression itself in UT #47401

[SPARK-48935][SQL][TESTS] Make `checkEvaluation` directly check the `Collation` expression itself in UT #47401

panbingkun commented Jul 18, 2024 •

edited

Loading

panbingkun Jul 18, 2024

panbingkun Jul 18, 2024

panbingkun commented Jul 18, 2024

cloud-fan Jul 18, 2024

panbingkun Jul 18, 2024

panbingkun Jul 18, 2024

cloud-fan Jul 18, 2024

panbingkun Jul 18, 2024

cloud-fan Jul 23, 2024

panbingkun Jul 23, 2024

cloud-fan commented Jul 24, 2024

	class StringType private(val collationId: Int) extends AtomicType with Serializable {
	/**
	* Support for Binary Equality implies that strings are considered equal only if
	* they are byte for byte equal. E.g. all accent or case-insensitive collations are considered
	* non-binary. If this field is true, byte level operations can be used against this datatype
	* (e.g. for equality and hashing).
	*/
	def supportsBinaryEquality: Boolean =
	CollationFactory.fetchCollation(collationId).supportsBinaryEquality

	def isUTF8BinaryCollation: Boolean =
	collationId == CollationFactory.UTF8_BINARY_COLLATION_ID

	def isUTF8BinaryLcaseCollation: Boolean =
	collationId == CollationFactory.UTF8_LCASE_COLLATION_ID

	protected def checkEvaluation(
	expression: => Expression, expected: Any, inputRow: InternalRow = EmptyRow): Unit = {
	// Make it as method to obtain fresh expression everytime.
	def expr = prepareEvaluation(expression)

	private def prepareEvaluation(expression: Expression): Expression = {
	val serializer = new JavaSerializer(new SparkConf()).newInstance()
	val resolver = ResolveTimeZone
	val expr = resolver.resolveTimeZones(replace(expression))

	protected def replace(expr: Expression): Expression = expr match {
	case r: RuntimeReplaceable => replace(r.replacement)

[SPARK-48935][SQL][TESTS] Make checkEvaluation directly check the Collation expression itself in UT #47401

[SPARK-48935][SQL][TESTS] Make checkEvaluation directly check the Collation expression itself in UT #47401

Conversation

panbingkun commented Jul 18, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panbingkun commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Jul 24, 2024

[SPARK-48935][SQL][TESTS] Make `checkEvaluation` directly check the `Collation` expression itself in UT #47401

[SPARK-48935][SQL][TESTS] Make `checkEvaluation` directly check the `Collation` expression itself in UT #47401

panbingkun commented Jul 18, 2024 •

edited

Loading