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

refactor(frontend): cleanup infer_type and align with pg #1931

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

xiangjinwu
Copy link
Contributor

@xiangjinwu xiangjinwu commented Apr 19, 2022

What's changed and what's your intention?

  • Organize exprs into groups: logical, comparison, arithmetic, temporal, string.
  • According to pg, the order of num_types should be int2->int4->int8->numeric->float4->float8.
  • Remove unnecessary helper functions. It is actually shorter and more readable without them. For example build_binary_funcs produces all combinations of exprs * arg1 * arg2 and requires the return types to be the same, which is only useful for cmp.
  • Implicit cast on operands are still not supported yet.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #1931 (6793bd4) into main (5160586) will decrease coverage by 0.01%.
The diff coverage is 99.22%.

@@            Coverage Diff             @@
##             main    #1931      +/-   ##
==========================================
- Coverage   70.94%   70.93%   -0.02%     
==========================================
  Files         617      617              
  Lines       79946    79870      -76     
==========================================
- Hits        56719    56652      -67     
+ Misses      23227    23218       -9     
Flag Coverage Δ
rust 70.93% <99.22%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/frontend/src/expr/type_inference.rs 97.37% <99.22%> (+1.22%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@xiangjinwu xiangjinwu force-pushed the rust-frontend-refactor-infer-type branch from 2818fc0 to 6793bd4 Compare April 19, 2022 06:53
@xiangjinwu xiangjinwu marked this pull request as ready for review April 19, 2022 06:54
@xiangjinwu xiangjinwu changed the title refactor(frontend): WIP cleanup infer_type and align with pg refactor(frontend): cleanup infer_type and align with pg Apr 19, 2022
src/frontend/src/expr/type_inference.rs Show resolved Hide resolved
E::Equal,
E::NotEqual,
E::LessThan,
E::LessThanOrEqual,
E::GreaterThan,
E::GreaterThanOrEqual,
E::In,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did In go away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In is a variadic function that cannot be handled by this func signature map lookup. It will be handled in a later refactor together with case when.
#1910 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

My fault to add it. It should not have been to put in this map.

@xiangjinwu xiangjinwu merged commit 4b9f4e8 into main Apr 19, 2022
@xiangjinwu xiangjinwu deleted the rust-frontend-refactor-infer-type branch April 19, 2022 09:27
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