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.

Sign up for GitHub

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

Jump to bottom

[DAPHNE-#493] SQLParser: Run Aggregation without grouping #509

Merged
merged 10 commits into from
Jun 6, 2023
Merged

[DAPHNE-#493] SQLParser: Run Aggregation without grouping #509

merged 10 commits into from
Jun 6, 2023

Conversation

tomschw
Copy link
Contributor

@tomschw tomschw commented Apr 25, 2023

Fixed problem:

  • To use aggregations in a SQL query we currently have to use some kind of grouping, aggregation over a whole column is therefore only possible with a workaround.

Changes:

  • Add test which reproduces the problem.
  • Implement functionality by using corresponding already existing operations (AggAll or for Count the NumRow Operation).
  • Change the name of the temporary column that gets created when converting to int matrix.

Fixes #493.

@pdamme
Copy link
Collaborator

pdamme commented Apr 25, 2023

Thanks @tomschw for this fix. I will have a close look by the end of the week.

@pdamme pdamme self-requested a review April 25, 2023 17:24
tomschw and others added 3 commits May 24, 2023 13:51
@tomschw
Merge branch 'daphne-eu:main' into 493-sql-agg-without-group
ab58631
@tomschw
Merge branch 'daphne-eu:main' into 493-sql-agg-without-group
0238ba6
@pdamme
Code simplifications and polishing.
39e90b8 
- SQLVisitor.cpp:
  - Removed helper function castToMatrixColumnWithOneEntry, since it can be replaced by a simple CastOp.
  - Simplified function visitGroupAggExpr a bit.
- Script-level test cases:
  - Renamed variable "lhs" (for "left-hand side") to "f" (for "frame"), since "left-hand side" doesn't make sense when there is no "right-hand side"...
pdamme
pdamme approved these changes Jun 6, 2023
View reviewed changes
Copy link
Collaborator

@pdamme pdamme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, @tomschw and sorry for the delay. The code looks very good to me and fixes the bug. I only slightly simplified the SQLVisitor (the helper function for creating a 1x1 matrix from a scalar via a FillOp could be replaced by just a CastOp; but admittedly only since a recent improvement to the type inference of CastOp in 19b2b44) and polished the script-level test cases a bit (for details see the commit I added).

@pdamme pdamme merged commit 81ccfdb into daphne-eu:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdamme pdamme pdamme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SQLParser: Run Aggregation without grouping
2 participants
@tomschw @pdamme