-
Notifications
You must be signed in to change notification settings - Fork 99
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 micro seconds precisison during copy unload #492
Open
arthurli1126
wants to merge
17
commits into
snowflakedb:master
Choose a base branch
from
arthurli1126:support-micro-seconds-during-copy-unload
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
91398eb
Support micro seconds precisison during copy unload
arthurli1126 52adb3e
Fixed typo
arthurli1126 285795a
Support copy unload in IT test timestamp
arthurli1126 0dedb0b
Fixed IT test
arthurli1126 371a675
Making sure the spark session exists for other test cases
arthurli1126 6151285
Added supportMicroSecs param and refactor IT test
arthurli1126 b373cd1
Merge branch 'master' into support-micro-seconds-during-copy-unload
arthurli1126 afed2e8
SNOW-760569 Bump spark connector and depenencies versions (#493)
sfc-gh-mrui ded0e0d
SNOW-760569 Upgrade to use JDBC 3.13.29 (#497)
sfc-gh-mrui f6b8e6c
SNOW-763124 Support uploading files with down-scoped token for Gcs (#…
sfc-gh-mrui 86a0fa9
SNOW-796952 Add option to disable pre- and post-action validation for…
sfc-gh-mrui 4698777
SNOW-770051 Fix a wrong result issue for crossing schema join/union (…
sfc-gh-mrui 21f4a50
SNOW-824475 Support Spark 3.4 (#510)
sfc-gh-mrui 0be43cc
SNOW-824420 Update SC version to 2.12.0 (#512)
sfc-gh-mrui 5e7bddd
pulled latest change from master
4de6d57
Merge branch 'support-micro-seconds-during-copy-unload' of github.com…
aaff19f
Fixed merge
arthurli1126 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -193,4 +193,88 @@ class ConversionsSuite extends FunSuite { | |
|
||
assert(expect == result.toString()) | ||
} | ||
|
||
test("Data with micro-seconds and nano-seconds precision should be correctly converted"){ | ||
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. This is a unit test. It proves that it can parse timestamp with micro/nano-seconds. |
||
val convertRow = Conversions.createRowConverter[Row](TestUtils.testSchema) | ||
val doubleMin = Double.MinValue.toString | ||
val longMax = Long.MaxValue.toString | ||
// scalastyle:off | ||
val unicodeString = "Unicode是樂趣" | ||
// scalastyle:on | ||
|
||
val timestampString = "2014-03-01 00:00:01.123456" | ||
|
||
val expectedTimestampMicro: Timestamp = java.sql.Timestamp.valueOf(timestampString) | ||
|
||
val dateString = "2015-07-01" | ||
val expectedDate = TestUtils.toMillis(2015, 6, 1, 0, 0, 0) | ||
|
||
|
||
|
||
val timestampString2 = "2014-03-01 00:00:01.123456789" | ||
|
||
val expectedTimestampMicro2: Timestamp = java.sql.Timestamp.valueOf(timestampString2) | ||
|
||
val dateString2 = "2015-07-01" | ||
val expectedDate2 = TestUtils.toMillis(2015, 6, 1, 0, 0, 0) | ||
|
||
val convertedRow = convertRow( | ||
Array( | ||
"1", | ||
dateString, | ||
"123.45", | ||
doubleMin, | ||
"1.0", | ||
"42", | ||
longMax, | ||
"23", | ||
unicodeString, | ||
timestampString | ||
) | ||
) | ||
|
||
val expectedRow = Row( | ||
1.asInstanceOf[Byte], | ||
new Date(expectedDate), | ||
new java.math.BigDecimal("123.45"), | ||
Double.MinValue, | ||
1.0f, | ||
42, | ||
Long.MaxValue, | ||
23.toShort, | ||
unicodeString, | ||
expectedTimestampMicro | ||
) | ||
|
||
val convertedRow2 = convertRow( | ||
Array( | ||
"1", | ||
dateString2, | ||
"123.45", | ||
doubleMin, | ||
"1.0", | ||
"42", | ||
longMax, | ||
"23", | ||
unicodeString, | ||
timestampString2 | ||
) | ||
) | ||
|
||
val expectedRow2 = Row( | ||
1.asInstanceOf[Byte], | ||
new Date(expectedDate2), | ||
new java.math.BigDecimal("123.45"), | ||
Double.MinValue, | ||
1.0f, | ||
42, | ||
Long.MaxValue, | ||
23.toShort, | ||
unicodeString, | ||
expectedTimestampMicro2 | ||
) | ||
|
||
assert(convertedRow == expectedRow) | ||
assert(convertedRow2 == expectedRow2) | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could you please add an internal parameter to enable/disable the change? It's enabled by default.
If some users is broken, they can disable the fix as a workaround.
For example, https://github.com/snowflakedb/spark-snowflake/blob/master/src/main/scala/net/snowflake/spark/snowflake/Parameters.scala#LL163C14-L163C14
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.
hmm.. do you mean to have a parameter such as
internal_support_micro_second_during_unload
? IMO, we should always try to support micro second level precision since by default direct JDBC supports this precision. It would make this part of the code confusing if we have two code paths and two set of timestamp patterns..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.
I agree users need to use this fix. This is why we can set the internal parameter as true by default.
This is the internal policy to introduce a parameter to disable the fix if possible.
The internal parameter can be removed later. So the code will be clean up accordingly.
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.
Thanks for the suggestion. Will push a new commit to add this parameter.