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

feat: Eliminate Duplicated Expr Rule #67

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Conversation

Sweetsuro
Copy link
Contributor

@Sweetsuro Sweetsuro commented Feb 14, 2024

Major Changes

Add an eliminate duplicate expression rule which:

  1. Removes duplicate sort expressions
  2. Removes duplicate aggregate group bys

Also, this PR adds derive traits for Hash, PartialEq, and Eq for RelNode.

Examples:
select * from t1 order by id, name, id desc, id asc, name desc becomes
select * from t1 order by id, name

select * from t1 group by id, name, id becomes
select * from t1 group by id, name

Rule Type

Heuristics (always apply), Transformation Rule (logical to logical)

@Sweetsuro Sweetsuro marked this pull request as ready for review February 15, 2024 04:46
@Sweetsuro Sweetsuro mentioned this pull request Feb 15, 2024
26 tasks
Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Rest LGTM

@skyzh
Copy link
Member

skyzh commented Feb 15, 2024

and adding a second rule that applies on the same expression might cause panic, at that time you should move the rule into the heuristics phase (need to create the heuristic optimizer in df-optimizer)

@skyzh
Copy link
Member

skyzh commented Feb 16, 2024

feel free to merge if you feel it's ready :)

@Sweetsuro Sweetsuro merged commit d34c22c into main Feb 16, 2024
1 check passed
@Sweetsuro Sweetsuro deleted the sweetsuro/eliminate_dup_expr branch February 16, 2024 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants