-
Notifications
You must be signed in to change notification settings - Fork 62
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
Initializes PartiQL Value classes #1091
Conversation
Conformance comparison report
Number passing in both: 5380 Number failing in both: 438 Number passing in Base (5b8a60f) but now fail: 0 Number failing in Base (5b8a60f) but now pass: 0 |
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
=========================================
Coverage 73.19% 73.19%
Complexity 2376 2376
=========================================
Files 237 237
Lines 17261 17261
Branches 3133 3133
=========================================
Hits 12635 12635
Misses 3653 3653
Partials 973 973
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
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.
One high level comment is that : Day Time Timestamp may need some future work, but it is out of the scope for this PR for sure.
Another comment I have is regarding the Reader APIs.
Thanks for putting this together.
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValueReader.kt
Outdated
Show resolved
Hide resolved
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValue.kt
Outdated
Show resolved
Hide resolved
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.
Only small nits to be fixed and perhaps the inclusion of an experimental annotation since these APIs will likely be broken in future releases. I'll leave the discussion regarding streams up to both you and @yliuuuu .
partiql-types/src/main/kotlin/org/partiql/value/PartiQLValueReader.kt
Outdated
Show resolved
Hide resolved
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.
This looks great to me. Thanks for the amazing work.
Description
This PR introduces an iteration of ExprValue (split from evaluator) informed by IonElement. Notably supporting annotations #1093 and defining Reader/Writer interfaces
Other Information
Updated Unreleased Section in CHANGELOG: [YES/NO]
NO
Any backward-incompatible changes? [YES/NO]
NO
Any new external dependencies? [YES/NO]
org.jetbrains.kotlinx:kotlinx-collections-immutable
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.