-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support multi string contains [databricks] #11413
Conversation
27654c6
to
4444aa4
Compare
Signed-off-by: Chong Gao <[email protected]>
4444aa4
to
3677ff6
Compare
Building failed, because it's depending on rapidsai/cudf#16641 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good and the performance numbers look good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nits
@@ -379,3 +379,30 @@ def test_case_when_all_then_values_are_scalars_with_nulls(): | |||
"tab", | |||
sql_without_else, | |||
conf = {'spark.rapids.sql.case_when.fuse': 'true'}) | |||
|
|||
@pytest.mark.parametrize('combine_string_contains_enabled', ['true', 'false']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer Python constants
@pytest.mark.parametrize('combine_string_contains_enabled', ['true', 'false']) | |
@pytest.mark.parametrize('combine_string_contains_enabled', [True, False]) |
However, the pytest case id will be more readable if, instead of a boolean, parameters are strings
@pytest.mark.parametrize('string_contains_mode', ['multiContains', 'singleContains'], ids=idfn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
override def equals(o: Any): Boolean = o match { | ||
case other: ContainsCombiner => exp.left.semanticEquals(other.exp.left) && | ||
exp.right.isInstanceOf[GpuLiteral] && other.exp.right.isInstanceOf[GpuLiteral] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you make ContainsCombiner a case class you can use pattern matching instead of manual instanceof checks:
override def equals(o: Any): Boolean = o match { | |
case other: ContainsCombiner => exp.left.semanticEquals(other.exp.left) && | |
exp.right.isInstanceOf[GpuLiteral] && other.exp.right.isInstanceOf[GpuLiteral] | |
override def equals(o: Any): Boolean = (o, exp) match { | |
case (ContainsCombiner(GpuContains(combLeft, GpuLiteral(_, _))), GpuContains(expLeft, GpuLiteral(_, _))) => | |
expLeft.semanticEquals(combLeft) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not significantly reduce the num of code lines, from 5 lines to 4 lines.
Let me keep the original versoin, since it's a nit.
The base branch was changed.
build |
1 similar comment
build |
The previous commit fixed the casting from String to UTF8String error when running
|
build |
Filed a follow-up issue optimize the multi-contains generated by rlike |
The perf test in the description was updated, please look again. |
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/stringFunctions.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/org/apache/spark/sql/rapids/stringFunctions.scala
Show resolved
Hide resolved
build |
Looks good to me. |
Combine multi targets for multiple string contains, and invoke one kernel to get multiple bool columns results.
Depends on cuDF PR
Verified:
contains
incase when
were combined when running pytest.Perf test
summary
details
short strings
The above part is kernel time of new kernel
The bottom part is base line.
long strings
Kernel time:
Base line:
new: