Spark 3.3: [FOLLOW UP] Support AS OF SPARK SQL syntax for TimeTravel E2E#5156
Conversation
|
|
||
| @Override | ||
| public Optional<String> extractTimeTravelVersion(CaseInsensitiveStringMap options) { | ||
| return Optional.ofNullable(PropertyUtil.propertyAsString(options, "versionAsOf", null)); |
There was a problem hiding this comment.
Where did these strings come from? Are they standard in Spark?
There was a problem hiding this comment.
I picked these string from the PR that introduced time travel in spark, but these doesn't seems to be a standard in Spark as there is no defined constants for the same.
To not conflict with existing options that are already in iceberg such as as-of-timestamp & SNAPSHOT_ID. Hence used the above.
| // AS OF expects the timestamp if given in long format will be of seconds precision | ||
| long timestampInSeconds = TimeUnit.MILLISECONDS.toSeconds(timestamp); | ||
| SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); | ||
| String formattedDate = sdf.format(new Date(timestamp)); |
There was a problem hiding this comment.
We generally prefer to use LocalDateTime and its associated format method, since java.util.Date has a time zone associated with it.
There was a problem hiding this comment.
makes sense, will make the changes :) !
|
I made a couple of comments about minor things, but overall I think this looks good and will commit when tests are passing. |
|
Thanks, @singhpk234! |
…5156) Co-authored-by: Prashant Singh <psinghvk@amazon.com>
…5156) Co-authored-by: Prashant Singh <psinghvk@amazon.com>
Closes :
About the Changes :
P.S This change would not have been possible without the awesome work done by @huaxingao in spark to support AS OF grammar in spark natively.
Testing :
Added new UT's
cc @rdblue, @aokolnychyi , @RussellSpitzer , @jackye1995 , @huaxingao , @flyrain