-
Notifications
You must be signed in to change notification settings - Fork 240
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
Fix order of operations when using mkString in typeConversionInfo #3113
Fix order of operations when using mkString in typeConversionInfo #3113
Conversation
Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/RapidsMeta.scala
Outdated
Show resolved
Hide resolved
build |
@andygrove do you have any other comments for this PR? |
s" but is going to be removed because $reasons" | ||
} | ||
|
||
private def typeConversionInfo: String = typeConversionReasons match { | ||
case None => "" | ||
case Some(v) if v.isEmpty => "" | ||
case Some(v) => | ||
" The data type of following expressions will be converted in GPU runtime:\n" + | ||
v mkString "; " | ||
"The data type of following expressions will be converted in GPU runtime: " + |
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: string interpolator can be used here?
val reasonMsg = (if (typeConverted) reason else None) | ||
.map(r => s", because $r").getOrElse("") | ||
s"Converted ${wrapped.getOrElse("N/A")} to " + | ||
s"${dataType.getOrElse("N/A")}" + reasonMsg |
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: same here we can use $reasonMsg inside the string
Sorry for the delay. LGTM. |
Prior to this, the explain was broken when doing type conversions (this was found while reviewing: #2971).
This tweaks the explain so it's in one line, and does some minor fixes. The original function duplicated the message, but I think the code I have here is safe since the
typeMeta
passed looks to be at the same expression level, and contains the same data as caller. It would be good to get confirmation I understood this correctly.