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

WIP Add support for recursive aggregate #2263

Closed
wants to merge 16 commits into from

Conversation

remysucre
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #2263 (917a36f) into master (bec3083) will decrease coverage by 0.22%.
The diff coverage is 32.67%.

❗ Current head 917a36f differs from pull request most recent head dd9c087. Consider uploading reports for the commit dd9c087 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2263      +/-   ##
==========================================
- Coverage   76.92%   76.69%   -0.23%     
==========================================
  Files         455      456       +1     
  Lines       28658    28638      -20     
==========================================
- Hits        22045    21964      -81     
- Misses       6613     6674      +61     
Impacted Files Coverage Δ
src/RelationTag.h 46.05% <0.00%> (-6.19%) ⬇️
src/parser/parser.yy 94.23% <0.00%> (-0.92%) ⬇️
src/ram/AggregateExistenceCheck.h 0.00% <0.00%> (ø)
src/ram/analysis/Complexity.cpp 90.90% <0.00%> (-9.10%) ⬇️
src/ram/analysis/Index.h 98.66% <ø> (ø)
src/ram/analysis/Level.cpp 59.16% <0.00%> (-2.58%) ⬇️
src/synthesiser/Synthesiser.cpp 81.78% <6.25%> (-1.35%) ⬇️
src/ram/analysis/Index.cpp 87.95% <7.69%> (-3.65%) ⬇️
src/ram/utility/Visitor.h 54.47% <50.00%> (-0.07%) ⬇️
src/synthesiser/Relation.cpp 95.17% <50.00%> (-3.04%) ⬇️
... and 15 more

@b-scholz b-scholz marked this pull request as ready for review April 25, 2022 23:30
@b-scholz b-scholz changed the title Add support for recursive aggregate WIP: Add support for recursive aggregate Apr 26, 2022
@b-scholz b-scholz changed the title WIP: Add support for recursive aggregate WIP Add support for recursive aggregate Apr 26, 2022
@b-scholz
Copy link
Member

I looked at the WIP again. As discussed, we need a semantic check for recursive-aggregate relations. I would check the following:

  • The arity of recursive-aggregate relations must be greater than or equal to two.
  • The type of the last attribute cannot be of type symbol if the aggregation is a sum
  • If the type of the last attribute is symbol, and the aggregation is min/max perhaps a warning should be issued that this is not the usual lexorder.

@b-scholz
Copy link
Member

Please bring the source files into the right format using clang-format -i <file>. Following source files are not formatted:

src/RelationTag.h
src/synthesiser/Relation.h

@b-scholz
Copy link
Member

Please also check whether I/O is working. The auxiliary arity of one may hide the last column.

@remysucre
Copy link
Author

Please also check whether I/O is working. The auxiliary arity of one may hide the last column.

I/O is working fine - all attributes are printed.

@quentin
Copy link
Member

quentin commented Jan 9, 2023

The technical choice of introducing new btree representations does not ease adding other aggregation operators in the future.

@b-scholz b-scholz closed this Jan 31, 2024
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.

None yet

3 participants