feat: Cast numeric (non int) to timestamp#3559
feat: Cast numeric (non int) to timestamp#3559coderfender wants to merge 4 commits intoapache:mainfrom
Conversation
coderfender
force-pushed
the
cast_float_to_timestamp
branch
from
41393a5 to
5aa49b0
Compare
coderfender
changed the title
coderfender
changed the title
coderfender
force-pushed
the
cast_float_to_timestamp
branch
from
5aa49b0 to
88e0c83
Compare
| * Uses except (difference) to find differences without using collect() | ||
| * Checks cometDF and sparkDF including schemas | ||
| */ | ||
| protected def assertDataFrameEquals( |
There was a problem hiding this comment.
In my recent PR , I had to cast back the generated I had to resort to this route because we have no native comet support from Timestamp -> Float causing operator / native comet check assertion failures . I tried casting back to String and Long either the cast is incompatible or unsupported on native side
| withTable("t1") { | ||
| generateDoubles().write.saveAsTable("t1") | ||
| val df = spark.sql("select a, cast(a as timestamp) from t1") | ||
| assertDataFrameEquals(df) |
There was a problem hiding this comment.
using assertDataFrameEquals instead of castTest means that we no longer have the ANSI and try_cast testing?
There was a problem hiding this comment.
Unfortunately that is true. But I could create a follow up to try and handle ANSI errors
There was a problem hiding this comment.
Comet currently has the correct ANSI behavior because it falls back to Spark. I don't think we should merge this PR until we have confirmed that removing the fallback isn't introducing any correctness issues.
There was a problem hiding this comment.
Sure. I totally agree that. I am working on introducing try and ANSI behavior to 'assertDataFrameEquals' method to make sure we don't skip any test case
coderfender
force-pushed
the
cast_float_to_timestamp
branch
from
bd49514 to
4d3a6a3
Compare
|
@andygrove I moved the 'assertDataFrameEquals' inside cast test to leverage existing Try and ANSI checks . This should help us gain confidence with native code and perhaps merge it |
2 participants
Which issue does this PR close?
Closes ##3560 .
Rationale for this change
Adding support for further more native cast support.
This should more or less close out the cast matrix in terms of support (barring other not planned casts such as Timestamp -> Int etc)
What changes are included in this PR?
How are these changes tested?
CometCastSuite.scalaand added new ones (along with benchmarking scripts) on the rust side to test cast at all Eval Modes