[ 
https://issues.apache.org/jira/browse/SPARK-47410?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Uroš Bojanić updated SPARK-47410:
---------------------------------
    Description: 
This ticket addresses the need to refactor the {{UTF8String}} and 
{{CollationFactory}} classes within Spark to enhance support for 
collation-aware expressions. The goal is to improve code structure, 
maintainability, readability, and testing coverage for collation-aware Spark 
SQL expressions.

The changed introduced herein should simplify addition of new collation-aware 
operations and ensure consistent testing across the codebase.

 

To further support the addition of collation support for new Spark expressions, 
here are a couple of guidelines to follow:

 

// 1. Collation-aware expression implementation

CollationSupport.java
 * should serve as a static entry point for collation-aware expressions, 
providing custom support
 * for example: one by one Spark expression with corresponding collation support
 * also note that: CollationAwareUTF8String should be used for collation-aware 
UTF8String operations & other utility methods

CollationFactory.java
 * should continue to serve as a static provider for high-level collation 
interface
 * for example: interacting with external ICU components such as Collator, 
StringSearch, etc.
 * also note that: no low-level / expression-specific code should generally be 
found here

UTF8String.java
 * should be largely collation-unaware, and generally be used only as storage, 
nothing else
 * for example: don’t change this class at all (with the only one-time 
exception of: semanticEquals/Compare)
 * also note that: no collation-aware operation implementations (using 
collationId) should be put here

stringExpressions.scala / regexpExpressions.scala / other 
“sql.catalyst.expressions” (for example: Between.scala)
 * should only contain minimal changes in order to re-route collation-aware 
implementations to CollationSupport
 * for example: most changes should be in relation to: adding collationId, 
using correct data types, replacements, etc.
 * also note that: nullSafeEval & doGenCode should likely note contain 
introduce extra branching based on collationId

 

// 2. Collation-aware expression testing

CollationSuite.scala
 * should be used for testing more general collation concepts
 * for example: collate/collation expressions, collation names, DDL, casting, 
aggregate, shuffle, join, etc.
 * also note that: no extra tests should generally be added

CollationSupportSuite.java
 * should be used for expression unit tests, these tests should be as rigorous 
as possible in order to cover various cases
 * for example: unit tests that test collation-aware expression implementation 
for various collations (binary, lowercase, ICU)
 * also note that: these tests should generally be written after adding 
appropriate expression support in CollationSupport.java

CollationStringExpressionsSuite.scala / CollationRegexpExpressionsSuite.scala / 
CollationExpressionSuite.scala
 * should be used for expression end-to-end tests, these tests should only 
cover crucial expression behaviour
 * for example: SQL tests that verify query execution results, expected return 
data types, casting, unsupported collation handling, etc.
 * also note that: these tests should generally be written after enabling 
appropriate expression support in stringExpressions.scala

 

// 3. Closing notes
 * Carefully think about performance implications of newly added custom 
collation-aware expression implementation
 * for example: be very careful with extra string allocations (UTF8Strings -> 
(Java) String -> UTF8Strings, etc.)
 * also note that: some operations introduce very heavy performance penalties 
(we should avoid the ones we can)

 
 * Make sure to test all newly added expressions and completely (unit tests, 
end-to-end tests, etc.)
 * for example: consider edge, such as: empty strings, uppercase and lowercase 
mix, different byte-length chars, etc.
 * also note that: all similar tests should be uniform & readable and be kept 
in one place for various expressions

 
 * Consider how new expressions interact with the rest of the system (casting; 
collation support level - use correct AbstractStringType, etc.)
 * for example: we should watch out for casting, test it thoroughly, and use 
CollationTypeCasts if needed for new expressions
 * also note that: some expressions won’t support all collation types, so that 
should be clearly reflected in tests and dataTypes

  was:
This ticket addresses the need to refactor the {{UTF8String}} and 
{{CollationFactory}} classes within Spark to enhance support for 
collation-aware expressions. The goal is to improve code structure, 
maintainability, readability, and testing coverage for collation-aware Spark 
SQL expressions.

The changed introduced herein should simplify addition of new collation-aware 
operations and ensure consistent testing across the codebase.

 

To further support the addition of collation support for new Spark expressions, 
here are a couple of guidelines to follow:

 

// 1. Collation-aware expression implementation

CollationSupport.java
 * should serve as a static entry point for collation-aware expressions, 
providing custom support
 * for example: one by one Spark expression with corresponding collation support
 * also note that: CollationAwareUTF8String should be used for collation-aware 
UTF8String operations & other utility methods

CollationFactory.java
 * should continue to serve as a static provider for high-level collation 
interface
 * for example: interacting with external ICU components such as Collator, 
StringSearch, etc.
 * also note that: no low-level / expression-specific code should generally be 
found here

UTF8String.java
 * should be largely collation-unaware, and generally be used only as storage, 
nothing else
 * for example: don’t change this class at all (with the only one-time 
exception of: semanticEquals/Compare)
 * also note that: no collation-aware operation implementations (using 
collationId) should be put here

stringExpressions.scala / regexpExpressions.scala / other 
“sql.catalyst.expressions” (for example: Between.scala)
 * should only contain minimal changes in order to re-route collation-aware 
implementations to CollationSupport
 * for example: most changes should be in relation to: adding collationId, 
using correct data types, replacements, etc.
 * also note that: nullSafeEval & doGenCode should likely note contain 
introduce extra branching based on collationId

 

// 2. Collation-aware expression testing

CollationSuite.scala
 * should be used for testing more general collation concepts
 * for example: collate/collation expressions, collation names, DDL, casting, 
aggregate, shuffle, join, etc.
 * also note that: no extra tests should generally be added

CollationSupportSuite.java
 * should be used for expression unit tests, these tests should be as rigorous 
as possible in order to cover various cases
 * for example: unit tests that test collation-aware expression implementation 
for various collations (binary, lowercase, ICU)
 * also note that: these tests should generally be written after adding 
appropriate expression support in CollationSupport.java

CollationStringExpressionsSuite.scala / CollationRegexpExpressionsSuite.scala / 
CollationExpressionSuite.scala
 * should be used for expression end-to-end tests, these tests should only 
cover crucial expression behaviour
 * for example: SQL tests that verify query execution results, expected return 
data types, casting, unsupported collation handling, etc.
 * also note that: these tests should generally be written after enabling 
appropriate expression support in stringExpressions.scala

 

// 3. Closing notes
 * Carefully think about performance implications of newly added custom 
collation-aware expression implementation
 * for example: be very careful with extra string allocations (UTF8Strings -> 
(Java) String -> UTF8Strings, etc.)
 * also note that: some operations introduce very heavy performance penalties 
(we should avoid the ones we can)
 * Make sure to test all newly added expressions and completely (unit tests, 
end-to-end tests, etc.)
 * for example: consider edge, such as: empty strings, uppercase and lowercase 
mix, different byte-length chars, etc.
 * also note that: all similar tests should be uniform & readable and be kept 
in one place for various expressions
 * Consider how new expressions interact with the rest of the system (casting; 
collation support level - use correct AbstractStringType, etc.)
 * for example: we should watch out for casting, test it thoroughly, and use 
CollationTypeCasts if needed for new expressions
 * also note that: some expressions won’t support all collation types, so that 
should be clearly reflected in tests and dataTypes


> refactor UTF8String and CollationFactory
> ----------------------------------------
>
>                 Key: SPARK-47410
>                 URL: https://issues.apache.org/jira/browse/SPARK-47410
>             Project: Spark
>          Issue Type: Sub-task
>          Components: SQL
>    Affects Versions: 4.0.0
>            Reporter: Uroš Bojanić
>            Priority: Major
>              Labels: pull-request-available
>
> This ticket addresses the need to refactor the {{UTF8String}} and 
> {{CollationFactory}} classes within Spark to enhance support for 
> collation-aware expressions. The goal is to improve code structure, 
> maintainability, readability, and testing coverage for collation-aware Spark 
> SQL expressions.
> The changed introduced herein should simplify addition of new collation-aware 
> operations and ensure consistent testing across the codebase.
>  
> To further support the addition of collation support for new Spark 
> expressions, here are a couple of guidelines to follow:
>  
> // 1. Collation-aware expression implementation
> CollationSupport.java
>  * should serve as a static entry point for collation-aware expressions, 
> providing custom support
>  * for example: one by one Spark expression with corresponding collation 
> support
>  * also note that: CollationAwareUTF8String should be used for 
> collation-aware UTF8String operations & other utility methods
> CollationFactory.java
>  * should continue to serve as a static provider for high-level collation 
> interface
>  * for example: interacting with external ICU components such as Collator, 
> StringSearch, etc.
>  * also note that: no low-level / expression-specific code should generally 
> be found here
> UTF8String.java
>  * should be largely collation-unaware, and generally be used only as 
> storage, nothing else
>  * for example: don’t change this class at all (with the only one-time 
> exception of: semanticEquals/Compare)
>  * also note that: no collation-aware operation implementations (using 
> collationId) should be put here
> stringExpressions.scala / regexpExpressions.scala / other 
> “sql.catalyst.expressions” (for example: Between.scala)
>  * should only contain minimal changes in order to re-route collation-aware 
> implementations to CollationSupport
>  * for example: most changes should be in relation to: adding collationId, 
> using correct data types, replacements, etc.
>  * also note that: nullSafeEval & doGenCode should likely note contain 
> introduce extra branching based on collationId
>  
> // 2. Collation-aware expression testing
> CollationSuite.scala
>  * should be used for testing more general collation concepts
>  * for example: collate/collation expressions, collation names, DDL, casting, 
> aggregate, shuffle, join, etc.
>  * also note that: no extra tests should generally be added
> CollationSupportSuite.java
>  * should be used for expression unit tests, these tests should be as 
> rigorous as possible in order to cover various cases
>  * for example: unit tests that test collation-aware expression 
> implementation for various collations (binary, lowercase, ICU)
>  * also note that: these tests should generally be written after adding 
> appropriate expression support in CollationSupport.java
> CollationStringExpressionsSuite.scala / CollationRegexpExpressionsSuite.scala 
> / CollationExpressionSuite.scala
>  * should be used for expression end-to-end tests, these tests should only 
> cover crucial expression behaviour
>  * for example: SQL tests that verify query execution results, expected 
> return data types, casting, unsupported collation handling, etc.
>  * also note that: these tests should generally be written after enabling 
> appropriate expression support in stringExpressions.scala
>  
> // 3. Closing notes
>  * Carefully think about performance implications of newly added custom 
> collation-aware expression implementation
>  * for example: be very careful with extra string allocations (UTF8Strings -> 
> (Java) String -> UTF8Strings, etc.)
>  * also note that: some operations introduce very heavy performance penalties 
> (we should avoid the ones we can)
>  
>  * Make sure to test all newly added expressions and completely (unit tests, 
> end-to-end tests, etc.)
>  * for example: consider edge, such as: empty strings, uppercase and 
> lowercase mix, different byte-length chars, etc.
>  * also note that: all similar tests should be uniform & readable and be kept 
> in one place for various expressions
>  
>  * Consider how new expressions interact with the rest of the system 
> (casting; collation support level - use correct AbstractStringType, etc.)
>  * for example: we should watch out for casting, test it thoroughly, and use 
> CollationTypeCasts if needed for new expressions
>  * also note that: some expressions won’t support all collation types, so 
> that should be clearly reflected in tests and dataTypes



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@spark.apache.org
For additional commands, e-mail: issues-h...@spark.apache.org

Reply via email to