-
Notifications
You must be signed in to change notification settings - Fork 61
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
Adds support for aggregations #1367
Conversation
Conformance comparison report-Cross Engine
Number failing in both: 269 Number passing in eval engine but fail in legacy engine: 169 Number failing in eval engine but pass in legacy engine: 1039 Conformance comparison report-Cross Commit-EVAL
Number failing in both: 1306 Number passing in Base (f1aeb6f) but now fail: 2 Number failing in Base (f1aeb6f) but now pass: 1055 Click here to see
Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 438 Number passing in Base (f1aeb6f) but now fail: 0 Number failing in Base (f1aeb6f) but now pass: 0 |
5ee9c0b
to
3a4a363
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## partiql-eval #1367 +/- ##
===============================================
Coverage ? 50.32%
Complexity ? 1045
===============================================
Files ? 165
Lines ? 13129
Branches ? 2452
===============================================
Hits ? 6607
Misses ? 5862
Partials ? 660
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9ec175b
to
40a065f
Compare
23c9e26
to
6e8d880
Compare
f945f87
to
45315e4
Compare
Adds support for COLL_AGGs
45315e4
to
1c7dce1
Compare
return userInputPath.steps.size + actualAbsolutePath.size - pathSentToConnector.steps.size | ||
} | ||
|
||
@OptIn(FnExperimental::class, PartiQLValueExperimental::class) |
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.
From here till EOF, this is copied from PathResolverAgg
.
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.
Essentially this whole file comes from org.partiql.lang.eval.physical.operators.Accumulator.kt
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.
Consider the query :
SELECT * FROM <<{'a': 1, 'b':2}, {'a':2, 'b':3}>> as tbl group by a
With the current implementation, this PR would throw an evaluationError.
The suspect cause is the NormalizedSelect Pass:
The NormalizeSelect Pass would rewrite the the query to be
SELECT VALUE TUPLEUNION(
CASE WHEN tbl IS STRUCT THEN tbl ELSE {_1: tbl}
FROM tbl GROUP BY a
)
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.
LGTM.
Relevant Issues
Description
NormalizeGroupBy
normalization pass.Unresolved Questions
Other Information
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.