-
Notifications
You must be signed in to change notification settings - Fork 28.5k
[SPARK-51421][SQL] Get seconds of TIME datatype #50525
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,4 +168,62 @@ class TimeExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper { | |
checkConsistencyBetweenInterpretedAndCodegen( | ||
(child: Expression) => MinutesOfTime(child).replacement, TimeType()) | ||
} | ||
|
||
test("SecondExpressionBuilder") { | ||
// Empty expressions list | ||
checkError( | ||
exception = intercept[AnalysisException] { | ||
SecondExpressionBuilder.build("second", Seq.empty) | ||
}, | ||
condition = "WRONG_NUM_ARGS.WITHOUT_SUGGESTION", | ||
parameters = Map( | ||
"functionName" -> "`second`", | ||
"expectedNum" -> "> 0", | ||
"actualNum" -> "0", | ||
"docroot" -> SPARK_DOC_ROOT) | ||
) | ||
|
||
// test TIME-typed child should build SecondsOfTime | ||
val timeExpr = Literal(localTime(12, 58, 59), TimeType()) | ||
val builtExprForTime = SecondExpressionBuilder.build("second", Seq(timeExpr)) | ||
assert(builtExprForTime.isInstanceOf[SecondsOfTime]) | ||
assert(builtExprForTime.asInstanceOf[SecondsOfTime].child eq timeExpr) | ||
|
||
// test non TIME-typed child should build second | ||
val tsExpr = Literal("2007-09-03 10:45:23") | ||
val builtExprForTs = SecondExpressionBuilder.build("second", Seq(tsExpr)) | ||
assert(builtExprForTs.isInstanceOf[Second]) | ||
assert(builtExprForTs.asInstanceOf[Second].child eq tsExpr) | ||
} | ||
|
||
test("Second with TIME type") { | ||
// A few test times in microseconds since midnight: | ||
// time in microseconds -> expected second | ||
val testTimes = Seq( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add these tests to test the output based on precision?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vinodkc Sure Vinod. I will create a function for SecondsOfTimeWithFraction and also include tests for the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @senthh , I updated the above comment; please check |
||
localTime() -> 0, | ||
localTime(1) -> 0, | ||
localTime(0, 59) -> 0, | ||
localTime(14, 30) -> 0, | ||
localTime(12, 58, 59) -> 59, | ||
localTime(23, 0, 1) -> 1, | ||
localTime(23, 59, 59, 999999) -> 59 | ||
) | ||
|
||
// Create a literal with TimeType() for each test microsecond value | ||
// evaluate SecondsOfTime(...), and check that the result matches the expected second. | ||
testTimes.foreach { case (micros, expectedSecond) => | ||
checkEvaluation( | ||
SecondsOfTime(Literal(micros, TimeType())), | ||
expectedSecond) | ||
} | ||
|
||
// Verify NULL handling | ||
checkEvaluation( | ||
SecondsOfTime(Literal.create(null, TimeType(TimeType.MICROS_PRECISION))), | ||
null | ||
) | ||
|
||
checkConsistencyBetweenInterpretedAndCodegen( | ||
(child: Expression) => SecondsOfTime(child).replacement, TimeType()) | ||
} | ||
} |
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.
Shouldn't it be DecimalType with a precision and scale that matches the precision of TimeType?
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.
@vinodkc In jira, @MaxGekk has given just IntegerType seconds for an example. So I thought the requirement is just to handle IntegerType. I can modify the implementation so that it should handle both with precision and without.
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.
Hi @vinodkc ,
MaxxGek has responded in jira that we need to return second without fraction. So as per Maxx requirement this PR will work without Fraction.
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.
And also test failed with "java.lang.OutOfMemoryError: Java heap space" is not relevant to our changes
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.
@HyukjinKwon @vinodkc and @MaxGekk It will be helpful if you re-review this PR and provide your input
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.
Yep, the type should be
IntergerType