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/12/13 23:41:16 UTC
[PR] [SPARK-49661][SQL]Removing redudant checks of collation in binary collation hashing expression [spark]
jovanpavl-db opened a new pull request, #49186:
URL: https://github.com/apache/spark/pull/49186
### What changes were proposed in this pull request?
Removing redudant check of collation in binary collation hashing expression
### Why are the changes needed?
Code cleanup, trying to speed up joins.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing tests should keep passing as join is already well covered with 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-49661][SQL]Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2542587237
> If this is really that performance critical, should we make supportsBinaryEquality a lazy val?
Can we improve it altogether? Right now it fetches the collator object and gets one of its properties. We can simply do integer comparisons against 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-49661][SQL] Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #49186: [SPARK-49661][SQL] Removing redudant checks of collation in binary collation hashing expression
URL: https://github.com/apache/spark/pull/49186
--
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-49661][SQL] Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2545702804
@stefankandic Are you ok with the changes?
--
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-49661][SQL] Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2550987421
> @stefankandic Are you ok with the changes?
I am now
--
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-49661][SQL]Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "jovanpavl-db (via GitHub)" <gi...@apache.org>.
jovanpavl-db commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2543556760
@stefankandic it is improvement, equally as using lazy val. By `stringType.collationId == CollationFactory.UTF8_BINARY_COLLATION_ID`, there is no fetching of collation at all (by collationId), just comparing 2 integers which should be pretty same as comparing boolean (supportsBinaryEquality with true).
Issue with current implementation is that we always need to fetch Collation from collationId to check simple boolean flag and that part of fetching Collation is the expensive one. But this is the way it worked before (adding rtrim collation), I added one more condition so it made it fetch 2 times (once for supportsBinaryEquality, once for supportsTrimCollation).
--
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-49661][SQL]Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2543097320
> > If this is really that performance critical, should we make supportsBinaryEquality a lazy val?
>
> Can we improve it altogether? Right now it fetches the collator object and gets one of its properties. We can simply do integer comparisons against it.
How is that an improvement though? If it were a val or a lazy val it would get computed only once, and now we have to do a comparison on each call, which for the interpreted path could happen billions of times.
--
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-49661][SQL] Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2551512282
+1, LGTM. Merging to master.
Thank you, @jovanpavl-db and @stefankandic @stevomitric 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-49661][SQL]Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "jovanpavl-db (via GitHub)" <gi...@apache.org>.
jovanpavl-db commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2542554440
@cloud-fan, @stefankandic, @stevomitric please review this urgently.
--
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-49661][SQL]Removing redudant checks of collation in binary collation hashing expression [spark]
Posted by "stefankandic (via GitHub)" <gi...@apache.org>.
stefankandic commented on PR #49186:
URL: https://github.com/apache/spark/pull/49186#issuecomment-2542585474
If this is really that performance critical, should we make `supportsBinaryEquality` a lazy val?
--
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