You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2024/10/22 11:22:50 UTC
[PR] [WIP][SPARK-50062][SQL] Support collations by `InSet` [spark]
MaxGekk opened a new pull request, #48598:
URL: https://github.com/apache/spark/pull/48598
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812662983
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
Review Comment:
renamed
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812533476
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +657,14 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType =>
+ if (st.supportsBinaryEquality) {
Review Comment:
Could you clarify why do you propose to restrict it to much?
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812596783
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
Review Comment:
I wouldn't agree that they are specific only to `InSet`, at least in my mind they are just collation aware implementations of the scala's set interface; so they can (and probably will) be used by someone other than `InSet`
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812698149
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
Review Comment:
The methods shouldn't be invoked at all. Let me raise an error here.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812660924
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
Review Comment:
renamed
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48598:
URL: https://github.com/apache/spark/pull/48598#issuecomment-2435218756
Merging to master. Thank you, @stefankandic @uros-db @cloud-fan for review.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812346938
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
This seems pretty inefficient, can we also do the similar thing as we did for the lcase one -> preprocess all elements by calculating the hashCode of the string's collationKey and then create a Map[Int, Seq[String]] or even better Set[CollationKey].
After that, in contains we can query the map to see if the element is present or not.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812765147
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
> This seems pretty inefficient
It depends on the cost of key calculation, size of the input set, how much are comparisons of the particular collation heavy. This could seems like "premature optimizations", but maybe you are right.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48598: [SPARK-50062][SQL] Support collations by `InSet`
URL: https://github.com/apache/spark/pull/48598
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812258235
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +657,14 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType =>
+ if (st.supportsBinaryEquality) {
Review Comment:
```suggestion
if (st.isUTF8BinaryCollation) {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +657,14 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType =>
+ if (st.supportsBinaryEquality) {
+ hset
+ } else if (st.supportsLowercaseEquality) {
Review Comment:
```suggestion
} else if (st.isUTF8LcaseCollation) {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
This seems pretty inefficient, can we also do the similar thing as we did for the lcase one -> preprocess all elements by calculating the hashCode of the string's collationKey and then create a Map[Int, Seq[String]].
After that, in contains we can query the map to see if the element is present or not.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
Review Comment:
can we create this variable outside of the function so we only fetch the collation once?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
cc: @uros-db who did some work with collation keys to see if this makes sense
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
Review Comment:
shouldn't these methods also return a `LCaseSet` (same for `excl` and both methods in the other class)?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
Review Comment:
this seems dangerous, can we implement Set[UTF8String] directly (and also take Set[UTF8String] in constructor?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
Review Comment:
since these classes seem pretty general I'd maybe think about moving them somewhere where other parts of the code can use them as well, something like CollationUtils maybe?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
Review Comment:
```suggestion
class LowercaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
Review Comment:
```suggestion
class CollationAwareSet(inputSet: Set[Any], collationId: Int)
```
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812558974
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
Review Comment:
I have tried this already, the problem is in the expected type `Set[Any]`
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1814494952
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +658,9 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType if !st.isUTF8BinaryCollation =>
Review Comment:
@cloud-fan I added additional checks for nested collated strings: array of strings, mixed collated strings in map of strings.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48598:
URL: https://github.com/apache/spark/pull/48598#issuecomment-2430059744
@stefankandic @c27kwan @cstavr @olaky @zhipengmao-db Please, review this PR.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812633025
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
Review Comment:
Till it sits inside of the scope of `InSet`, it doesn't matter from my point of view. But as soon as you move it out to some common place like `CollationUtils`, you cannot make any assumptions. For example:
1. `InSet` guarantees non-null inputs, and we will have to add explicit checks
2. `InSet` assumes `set` is serializable. So, you have to make it such.
3. Properly implement other methods besides of the default constructor and `contains()`. `InSet` doesn't need them.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812643519
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
Review Comment:
> so they can (and probably will) be used by someone other than InSet
Is is called "Premature generalization". I would prefer to avoid the anti-pattern if it is possible. Let's make the cases more generic when it is really needed.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812530741
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
Review Comment:
The class should be serializable. If you move this out, you will hit a serialization issue.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812542404
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
Review Comment:
> since these classes seem pretty general
hmm why? So far the classes are pretty specific to `InSet`. I didn't get why do you want to make them too generic.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1814433450
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +658,9 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType if !st.isUTF8BinaryCollation =>
Review Comment:
Yep, in that case `InSet` uses `TreeSet` to which we pass an `Ordering` instance. The last one is aware of collations via
https://github.com/apache/spark/blob/aeddc3f55e604616d683bdcc3ce7c21c95b018f6/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/PhysicalDataType.scala#L46
Let me add a few checks for nested collated strings.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812741524
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +657,14 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType =>
+ if (st.supportsBinaryEquality) {
+ hset
+ } else if (st.supportsLowercaseEquality) {
Review Comment:
@stefankandic Correct me if I am wrong. I need particular property: before to compare, I have to call `UTF8String.lowerCaseCodePoints` for both arguments. I guess there could be more than one collation that satisfies the property.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1814054928
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -655,6 +658,9 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
}
@transient lazy val set: Set[Any] = child.dataType match {
+ case st: StringType if !st.isUTF8BinaryCollation =>
Review Comment:
What if it's array of collated string? Shall we use `TreeSet` with collation aware ordering?
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812346938
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
This seems pretty inefficient, can we also do the similar thing as we did for the lcase one -> preprocess all elements by calculating the hashCode of the string's collationKey and then create a Map[Int, Seq[String]] or even better Set[CollationKey] directly.
After that, in contains we can query the map to see if the element is present or not.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812597781
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
Review Comment:
it looks ugly sure, but I think it's better if the caller handles it
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1813294016
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
Review Comment:
Moved the casting to the caller side though it is in the same expression/scope.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1813298400
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
I changed implementation, but I think the old implementation can overperform new one in some cases. Need to benchmark both.
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812564985
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
Review Comment:
In any case, need to case somewhere. If I follow your proposal, I have to cast at the caller side:
```
new InSet.LCaseSet(hset.asInstanceOf[Set[UTF8String]]).asInstanceOf[Set[Any]]
```
--
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-50062][SQL] Support collations by `InSet` [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48598:
URL: https://github.com/apache/spark/pull/48598#discussion_r1812371784
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala:
##########
@@ -767,6 +777,35 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
override protected def withNewChildInternal(newChild: Expression): InSet = copy(child = newChild)
}
+object InSet {
+ class LCaseSet(inputSet: Set[Any]) extends immutable.Set[Any] with Serializable {
+ private val strSet = inputSet.map { s =>
+ if (s == null) null
+ else CollationAwareUTF8String.lowerCaseCodePoints(s.asInstanceOf[UTF8String])
+ }
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ strSet.contains(CollationAwareUTF8String.lowerCaseCodePoints(elem.asInstanceOf[UTF8String]))
+ }
+ }
+ class CollationSet(inputSet: Set[Any], collationId: Int)
+ extends immutable.Set[Any] with Serializable {
+ override def incl(elem: Any): Set[Any] = inputSet.incl(elem)
+ override def excl(elem: Any): Set[Any] = inputSet.excl(elem)
+ override def iterator: Iterator[Any] = inputSet.iterator
+ override def contains(elem: Any): Boolean = {
+ assert(elem != null, "InSet guarantees non-null input")
+ val collation = CollationFactory.fetchCollation(collationId)
+ inputSet.exists { p =>
Review Comment:
yeah, for lots of comparisons we should use collation key
https://unicode-org.github.io/icu/userguide/collation/concepts#sortkeys-vs-comparison
--
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