Skip to content
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

Merged
merged 13 commits into from
Jun 21, 2023
Merged

Initializes PartiQL Value classes #1091

merged 13 commits into from
Jun 21, 2023

Conversation

RCHowell
Copy link
Contributor

@RCHowell RCHowell commented May 22, 2023

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.

@RCHowell RCHowell requested a review from johnedquinn May 22, 2023 20:09
@github-actions
Copy link

github-actions bot commented May 22, 2023

Conformance comparison report

Base (5b8a60f) 7ad330d +/-
% Passing 92.47% 92.47% 0.00%
✅ Passing 5380 5380 0
❌ Failing 438 438 0
🔶 Ignored 0 0 0
Total Tests 5818 5818 0

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

@RCHowell RCHowell changed the base branch from v1.0.0-dev to main May 24, 2023 21:43
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (d5a4ac4) 73.19% compared to head (c70c53d) 73.19%.

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           
Flag Coverage Δ
CLI 14.28% <ø> (ø)
EXAMPLES 80.28% <ø> (ø)
LANG 79.10% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@RCHowell RCHowell marked this pull request as ready for review June 15, 2023 21:26
@RCHowell RCHowell changed the title Initializes interfaces for PartiQL Value classes Initializes PartiQL Value classes Jun 15, 2023
Copy link
Contributor

@yliuuuu yliuuuu left a 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.

Copy link
Member

@johnedquinn johnedquinn left a 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 .

Copy link
Contributor

@yliuuuu yliuuuu left a 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.

@RCHowell RCHowell requested a review from johnedquinn June 21, 2023 22:47
@RCHowell RCHowell merged commit dc4020e into main Jun 21, 2023
@RCHowell RCHowell deleted the partiql-values branch June 21, 2023 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants