You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stevomitric (via GitHub)" <gi...@apache.org> on 2025/01/17 11:32:18 UTC
[PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
stevomitric opened a new pull request, #49554:
URL: https://github.com/apache/spark/pull/49554
### What changes were proposed in this pull request?
Added all RTRIM collations to TVF when using `Collations` generator.
### Why are the changes needed?
As per the collation project plan.
### Does this PR introduce _any_ user-facing change?
Yes, using the `collations` function will include `rtrim` collations.
### How was this patch tested?
Modified TVF collation tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #49554:
URL: https://github.com/apache/spark/pull/49554#discussion_r1922022551
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -689,7 +699,18 @@ static List<CollationIdentifier> listCollations() {
CollationNames.UTF8_LCASE,
CollationSpecICU.ICU_VERSION
);
- return Arrays.asList(UTF8_BINARY_COLLATION_IDENT, UTF8_LCASE_COLLATION_IDENT);
+ CollationIdentifier UTF8_BINARY_RTRIM_COLLATION_IDENT = new CollationIdentifier(
+ PROVIDER_SPARK,
+ CollationNames.UTF8_BINARY + "_RTRIM",
+ CollationSpecICU.ICU_VERSION
+ );
+ CollationIdentifier UTF8_LCASE_RTRIM_COLLATION_IDENT = new CollationIdentifier(
+ PROVIDER_SPARK,
+ CollationNames.UTF8_LCASE + "_RTRIM",
+ CollationSpecICU.ICU_VERSION
+ );
+ return Arrays.asList(UTF8_BINARY_COLLATION_IDENT, UTF8_LCASE_COLLATION_IDENT,
+ UTF8_BINARY_RTRIM_COLLATION_IDENT, UTF8_LCASE_RTRIM_COLLATION_IDENT);
Review Comment:
fix indentation. this is even inconsistent to other parts of the file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #49554: [SPARK-50875][SQL] Add RTRIM collations to TVF
URL: https://github.com/apache/spark/pull/49554
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #49554:
URL: https://github.com/apache/spark/pull/49554#discussion_r1920344710
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -460,6 +469,8 @@ static CollationMeta loadCollationMeta(CollationIdentifier collationIdentifier)
}
return collationSpecUTF8;
}
+
+ protected SpaceTrimming spaceTrimming;
Review Comment:
It is used in other instances to determine trimming. We can followup with a rename of TRIM collation to PAD (?).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #49554:
URL: https://github.com/apache/spark/pull/49554#issuecomment-2599420439
let's fix the python test failure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #49554:
URL: https://github.com/apache/spark/pull/49554#issuecomment-2598164875
cc @stefankandic and @dejankrak-db to take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #49554:
URL: https://github.com/apache/spark/pull/49554#issuecomment-2603612480
thanks, merging to master/4.0!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-50875][SQL] Add RTRIM collations to TVF [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #49554:
URL: https://github.com/apache/spark/pull/49554#discussion_r1920130238
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -460,6 +469,8 @@ static CollationMeta loadCollationMeta(CollationIdentifier collationIdentifier)
}
return collationSpecUTF8;
}
+
+ protected SpaceTrimming spaceTrimming;
Review Comment:
can we address this in a follow up at least? Feels weird seeing trim and pad used interchangeably
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org