-
Notifications
You must be signed in to change notification settings - Fork 12
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
Initial integration tests #34
Conversation
Successful test cases for type mapping with a jdbc connection and writing to local file system #32
|
||
@JvmStatic | ||
fun hive(): Hive { | ||
if (hive == null || hive!!.isClosed()) { |
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.
Note that I'm new to Kotlin. Do you need the !!
even though you've already short-circuited if hive == null
?
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.
Normally you don't need to, but since hive
is a var
Kotlin can't auto cast from Hive?
to Hive
after the check since it has no guarantees that the value didn't change
|
||
/** | ||
* Reads an Ion stream into the DOM model skipping null values instead of creating IonNull objects. This is necessary | ||
* for primitives since Hive handles the null check on their side and will not consider IonNull as java null |
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.
Does this mean there is no way to annotate a null or have a null with a field name?
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.
With #13 we'll add support to write out null
values explicitly. What we won't be able to do is preserve the original document distinction between not having the field or having the field with a null
value, it'll have to be all or nothing
Examples to make it more clear:
Hive table: (field Int)
// Supported
read: {}, {field: null}
write: {}, {}
// Supported with #13 as a configuration option
read: {}, {field: null}
write: {field: null.int} , {field: null.int}
// Not supported
read: {}, {field: null}
write: {} , {field: null.int}
Annotation support is still TBD and will not be included in v1, but there are some thoughts on #6. It should be possible as it still supports reading a null
field or an annotated null
field. It'll require the configuration introduced in #13 to be used or the null
value won't even be serialized
val floats = listOf( | ||
listOf("FLOAT", "-1e0"), | ||
listOf("FLOAT", "0e0"), | ||
listOf("FLOAT", "1e0") |
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.
Are nan, +inf, and -inf supported?
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.
just added them and hive does support it. Will add those three test cases before merging
...tion-test/src/test/kotlin/software/amazon/ionhiveserde/integrationtest/SuccessfulJdbcTest.kt
Show resolved
Hide resolved
Manually merged |
Successful test cases for type mapping with a jdbc connection and
writing to local file system
Issue #, if available:
#32
Description of changes:
Adds initial integration tests. More test cases must be added for all types and needs work to speed up the test execution. Separate issues will be open for those
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.