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

For avg functions replacing it with (sum / count); count(*) #415

Merged
merged 13 commits into from
Jan 4, 2024

Conversation

loloxwg
Copy link
Member

@loloxwg loloxwg commented Jan 4, 2024

What problem does this PR solve?

For avg functions replacing it with (sum / count)

The commit involves converting avg function expressions to (sum / count) function expressions in the SQL planner and test updates accordingly. Tests cover both the regular and exceptional cases. Corresponding changes are reflected in other parts of the code like ProjectBinder and Aggregate operators.

support count(*)

Issue link: #357

What is changed and how it works?

Code changes

  • Has Code change
  • Has CI related scripts change

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Note for reviewer

loloxwg added 13 commits January 4, 2024 16:26
This commit introduces the execution functionality in the PhysicalMergeAggregate operator. It provides data procedures for various aggregate functions like COUNT, MIN, MAX, and SUM across different DataTypes. Additionally, proper handling for input and Output dataBlocks has been implemented. Consequently, various changes to the TaskScheduler, code refactoring and logging adjustments were also done.
The PhysicalMergeAggregate operator execution logic has been updated to correctly handle COUNT, MIN, MAX, and SUM aggregate functions for different DataTypes. This commit also implements appropriate handling for input and output dataBlocks. Concurrently, various changes are performed in the TaskScheduler. Several code sections were refactored for better readability and logs have been properly adjusted for detailed tracing.
Deleted several unused template functions in `physical_merge_aggregate.cppm` and `physical_merge_aggregate.cpp`. Also removed a sizable chunk of unused logic related to execution of different aggregate functions from `PhysicalMergeAggregate::SimpleMergeAggregateExecute()` method. This declutters the codebase, making it easier to read and understand.
The PhysicalMergeAggregate class was refactored by introducing a new method, SimpleMergeAggregateExecute, to encapsulate some of the implementation details. The newly defined method simplifies the complexity of the code and makes the execute method easier to understand. ExpressionBinder class was also modified to improve clarity of the division function expression building process, simplifying it and making code more maintainable. Minor tweaks were also made to the fragment_context.cpp file to further enhance readability.
The commit involves converting avg function expressions to (sum / count) function expressions in the SQL planner and test updates accordingly. Tests cover both the regular and exceptional cases. Corresponding changes are reflected in other parts of the code like ProjectBinder and Aggregate operators.
The conversion of AVG function to SUM / COUNT has been refactored for efficiency and readability. A helper function, ConvertAvgToSumDivideCount, has been introduced to modularize this logic. The updated code significantly shrinks the BuildExpression method, improving its maintainability.
Modified the div functions to return results as Double type instead of previous specific integer types. Additionally, adjusted relevant test cases to match the changes. This allows for more accurate division results across the codebase.
A "count(*)" functionality was implemented, enabling users to count the number of rows of SQL tables. Several test scripts were modified to include a "SELECT count(*)" command immediately after table operations to verify the row count. The 'Update' and 'Delete' operations were adjusted in 'table_collection_entry.cpp', and 'StateUpdate' was modified in 'aggregate_function.cppm'. New 'CountStar' related files were added to implement the new functionality.
Amended the physical_merge_aggregate and related files for added traceability and support for new aggregate functions. Added handling for tinyInt and smallInt types and expanded test coverage in the generate_aggregate.py script. Redundant codes were also removed.
@JinHai-CN JinHai-CN added the ci PR can be test label Jan 4, 2024
@JinHai-CN
Copy link
Contributor

@mergify queue

Copy link

mergify bot commented Jan 4, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 98c3bcc

@JinHai-CN JinHai-CN self-requested a review January 4, 2024 09:42
@mergify mergify bot merged commit 98c3bcc into infiniflow:main Jan 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci PR can be test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants