You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "jovanpavl-db (via GitHub)" <gi...@apache.org> on 2024/10/15 08:28:21 UTC
[PR] Fix semantic of support binary equality [spark]
jovanpavl-db opened a new pull request, #48472:
URL: https://github.com/apache/spark/pull/48472
### What changes were proposed in this pull request?
With introduction of trim collation, what was known as supportsBinaryEquality changes, it is now split in isUtf8BinaryType and usesTrimCollation so that it has correct semantics.
### Why are the changes needed?
With introduction of trim collation, what was known as supportsBinaryEquality changes, it is now split in isUtf8BinaryType and usesTrimCollation so that it has correct semantics.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Everything is covered with existing tests, no new functionality is added.
### 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] Fix semantic of support binary equality [spark]
Posted by "jovanpavl-db (via GitHub)" <gi...@apache.org>.
jovanpavl-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800838010
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
Yes.
--
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] Fix semantic of support binary equality [spark]
Posted by "jovanpavl-db (via GitHub)" <gi...@apache.org>.
jovanpavl-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800802607
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
By incorporating `usesTrimCollation` flag we agreed on in the previous prs.
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800850453
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
sgtm
--
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-49911][SQL] Fix semantic of support binary equality [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #48472: [SPARK-49911][SQL] Fix semantic of support binary equality
URL: https://github.com/apache/spark/pull/48472
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800749343
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -168,24 +179,22 @@ public Collation(
String version,
ToLongFunction<UTF8String> hashFunction,
BiFunction<UTF8String, UTF8String, Boolean> equalsFunction,
- boolean supportsBinaryEquality,
- boolean supportsBinaryOrdering,
- boolean supportsLowercaseEquality,
+ boolean isUtf8BinaryType,
+ boolean isUtf8LcaseType,
boolean supportsSpaceTrimming) {
this.collationName = collationName;
this.provider = provider;
this.collator = collator;
this.comparator = comparator;
this.version = version;
this.hashFunction = hashFunction;
- this.supportsBinaryEquality = supportsBinaryEquality;
- this.supportsBinaryOrdering = supportsBinaryOrdering;
- this.supportsLowercaseEquality = supportsLowercaseEquality;
+ this.isUtf8BinaryType = isUtf8BinaryType;
+ this.isUtf8LcaseType = isUtf8LcaseType;
this.equalsFunction = equalsFunction;
this.supportsSpaceTrimming = supportsSpaceTrimming;
-
- // De Morgan's Law to check supportsBinaryOrdering => supportsBinaryEquality
- assert(!supportsBinaryOrdering || supportsBinaryEquality);
+ this.supportsBinaryEquality = !supportsSpaceTrimming && isUtf8BinaryType;
+ this.supportsBinaryOrdering = !supportsSpaceTrimming && isUtf8BinaryType;
+ this.supportsLowercaseEquality = !supportsSpaceTrimming && isUtf8LcaseType;
Review Comment:
```suggestion
this.supportsBinaryEquality = isUtf8BinaryType;
this.supportsBinaryOrdering = isUtf8BinaryType;
this.supportsLowercaseEquality = isUtf8LcaseType;
// Space trimmming.
this.supportsBinaryEquality &= !supportsSpaceTrimming;
this.supportsBinaryOrdering &= !supportsSpaceTrimming;
this.supportsLowercaseEquality &= !supportsSpaceTrimming;
```
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800829988
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
so the idea is:
1. apply the space trimming policy to the arguments, using the `usesTrimCollation` flag
2. proceed with `isUtf8BinaryType` / `isUtf8LcaseType` check (which is _less_ restrictive than the previously used `supportsBinaryEquality` / `supportsLowercaseEquality` check)
and this finally gives us the proper & desired behaviour for UTF8_BINARY_RTRIM / UTF8_LCASE_RTRIM / UNICODE_RTRIM
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800829988
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
so the idea is:
1. apply trimming policy to the arguments
2. proceed with `isUtf8BinaryType` / `isUtf8LcaseType` check (which is _less_ restrictive than the previously used `supportsBinaryEquality` / `supportsLowercaseEquality` check)
and this finally gives us the proper & desired behaviour for UTF8_BINARY_RTRIM
--
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-49911][SQL] Fix semantic of support binary equality [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #48472:
URL: https://github.com/apache/spark/pull/48472#issuecomment-2414736702
+1, LGTM. Merging to master.
Thank you, @jovanpavl-db and @uros-db 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] Fix semantic of support binary equality [spark]
Posted by "jovanpavl-db (via GitHub)" <gi...@apache.org>.
jovanpavl-db commented on PR #48472:
URL: https://github.com/apache/spark/pull/48472#issuecomment-2413376448
> so let me check if I understand correctly
>
> we keep `supportsBinaryEquality`, which will be:
>
> * `true` for `UTF8_BINARY`
> * `false` for `UTF8_BINARY_RTRIM`
>
> and we also introduce `isUtf8BinaryType`, which will be:
>
> * `true` for `UTF8_BINARY`
> * `true` for `UTF8_BINARY_RTRIM`
>
> then, we refactor the execution-related code path branches to rely on `isUtf8BinaryType`, rather than `supportsBinaryEquality`?
Everything is correct except the last sentence. We will keep relying on `supportsBinaryEquality` in most cases (see for example hashing and aggregate). We will use `isUtf8BinaryType` when we don't want to have different behaviour for `UTF8_BINARY_RTRIM` and `UTF8_BINARY` collations.
--
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] Fix semantic of support binary equality [spark]
Posted by "jovanpavl-db (via GitHub)" <gi...@apache.org>.
jovanpavl-db commented on PR #48472:
URL: https://github.com/apache/spark/pull/48472#issuecomment-2413469284
> I see, this makes sense - I'm on board with the approach
>
> one small ask for you tho - let's hold off all unnecessary changes in this PR (for example: modifying the branching pattern in CollationSupport classes). I don't like the idea of modifying some code (that the execution flow can't reach anyways at this time, because analysis will block it) and not adding the appropriate tests
>
> please opt for one of the following:
>
> 1. undo all needless & untested changes here, and only keep the minimally required scaffolding for further implementation
> 2. keep the `supportsBinaryEquality` -> `isUtf8BinaryType` changes, but also apply the proper space trimming policy, whitelist the expression and add corresponding tests
>
> so let's just avoid half-baked code, it can be confusing both for you and the reviewers
Agree, makes sense. Let's go with 1, i.e we want have any changes without tests. Will (today) follow up with prs that will have tests and use that code path.
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800753444
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
iiuc, this is just renaming currently
but how exactly do we plan to modify CollationSupport classes, so that we get the desired behaviour regarding space trimming integration?
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800753900
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationFactory.java:
##########
@@ -153,13 +153,24 @@ public static class Collation {
* expressions, as this particular collation is not supported by the external ICU library.
*/
public final boolean supportsLowercaseEquality;
-
Review Comment:
undo
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800829988
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
so the idea is:
1. apply trimming policy to the arguments, using the `usesTrimCollation` flag
2. proceed with `isUtf8BinaryType` / `isUtf8LcaseType` check (which is _less_ restrictive than the previously used `supportsBinaryEquality` / `supportsLowercaseEquality` check)
and this finally gives us the proper & desired behaviour for UTF8_BINARY_RTRIM / UTF8_LCASE_RTRIM / UNICODE_RTRIM
--
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] Fix semantic of support binary equality [spark]
Posted by "uros-db (via GitHub)" <gi...@apache.org>.
uros-db commented on code in PR #48472:
URL: https://github.com/apache/spark/pull/48472#discussion_r1800829988
##########
common/unsafe/src/main/java/org/apache/spark/sql/catalyst/util/CollationSupport.java:
##########
@@ -37,9 +37,9 @@ public final class CollationSupport {
public static class StringSplitSQL {
public static UTF8String[] exec(final UTF8String s, final UTF8String d, final int collationId) {
CollationFactory.Collation collation = CollationFactory.fetchCollation(collationId);
- if (collation.supportsBinaryEquality) {
+ if (collation.isUtf8BinaryType) {
return execBinary(s, d);
- } else if (collation.supportsLowercaseEquality) {
+ } else if (collation.isUtf8LcaseType) {
Review Comment:
so the idea is:
1. apply trimming policy to the arguments
2. proceed with `isUtf8BinaryType` / `isUtf8LcaseType` check (which is _less_ restrictive than the previously used `supportsBinaryEquality` / `supportsLowercaseEquality` check)
and this finally gives us the proper & desired behaviour for UTF8_BINARY_RTRIM / UTF8_LCASE_RTRIM / UNICODE_RTRIM
--
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