-
Notifications
You must be signed in to change notification settings - Fork 244
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 casting between day-time interval and string #5155
Conversation
Signed-off-by: Chong Gao <[email protected]>
sql-plugin/src/main/330+/scala/com/nvidia/spark/rapids/shims/GpuIntervalUtils.scala
Outdated
Show resolved
Hide resolved
val numRows = micros.getRowCount | ||
val from = DT.fieldToString(startField).toUpperCase | ||
val to = DT.fieldToString(endField).toUpperCase | ||
val prefixStr = "INTERVAL '" |
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.
We can has this prefixStr
as a const member of GpuIntervalUtils, instead of creating one each time this method is called.
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
// prefix part: INTERVAL ' | ||
parts += getConstStringVector(prefixStr, numRows) | ||
|
||
// sign part: - or empty |
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.
We may merge the sign vector and the prefix vector into one vector, to reduce the vectors to be concatenated.
withResource(getConstStringVector("INTERVAL '-", numRows)) { neg =>
withResource(getConstStringVector("INTERVAL '", numRows)) { empty =>
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
/** | ||
* return (micros / div).toString | ||
*/ | ||
private def devResult(micros: ColumnVector, div: Long): ColumnVector = { |
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.
devResult -> divResult ?
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
/** | ||
* return (micros / div).toString with padding zero | ||
*/ | ||
private def devResultWithPadding(micros: ColumnVector, div: Long): ColumnVector = { |
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.
devResultWithPadding -> divResultWithPadding
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
Signed-off-by: Chong Gao <[email protected]>
Depending on #5020 to merge. |
// `restHolder` only hold one rest Cv; | ||
// use `resetRest` to close the old one and set a new one | ||
// make a copy of micros | ||
|
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.
// `restHolder` only hold one rest Cv; | |
// use `resetRest` to close the old one and set a new one | |
// make a copy of micros |
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
withResource(getConstStringVector(prefixStr + "-", numRows)) { neg => | ||
withResource(getConstStringVector(prefixStr, numRows)) { empty => | ||
less.ifElse(neg, empty) | ||
} | ||
} |
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.
ifElse
supports operating on scalar values directly which is more efficient, as we don't need to manifest a column of identical scalars.
withResource(getConstStringVector(prefixStr + "-", numRows)) { neg => | |
withResource(getConstStringVector(prefixStr, numRows)) { empty => | |
less.ifElse(neg, empty) | |
} | |
} | |
withResource(Scalar.fromString(prefixStr + "-")) { negPrefix => | |
withResource(Scalar.fromString(prefixStr)) { prefix => | |
less.ifElse(negPrefix, prefix) | |
} | |
} |
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
build |
@@ -120,4 +124,5 @@ object GpuTypeShims { | |||
*/ | |||
def additionalCommonOperatorSupportedTypes: TypeSig = TypeSig.none | |||
|
|||
def isDayTimeType(t: DataType): Boolean = 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.
The name here is problematic on Spark 3.2.x which does support DayTimeIntervalType
and could return false when called with a DayTimeIntervalType
instance. Maybe this should be named something like isSupportedDayTimeType
?
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, already have isSupportedDayTimeType
.
tests/src/test/330/scala/com/nvidia/spark/rapids/GpuIntervalUtilsTest.scala
Outdated
Show resolved
Hide resolved
tests/src/test/330/scala/com/nvidia/spark/rapids/GpuIntervalUtilsTest.scala
Outdated
Show resolved
Hide resolved
build |
Contributes #4811
Signed-off-by: Chong Gao [email protected]