Skip to content

feat(expr): support interval * int#1663

Merged
TennyZhuang merged 5 commits intomainfrom
feat/interval-mul-int
Apr 7, 2022
Merged

feat(expr): support interval * int#1663
TennyZhuang merged 5 commits intomainfrom
feat/interval-mul-int

Conversation

@TennyZhuang
Copy link
Contributor

Signed-off-by: TennyZhuang zty0826@gmail.com

What's changed and what's your intention?

As title

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)

#112

Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@github-actions github-actions bot added the type/feature Type: New feature. label Apr 7, 2022
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@TennyZhuang TennyZhuang enabled auto-merge (squash) April 7, 2022 09:59
@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #1663 (47b6b1a) into main (bf74823) will decrease coverage by 0.01%.
The diff coverage is 39.47%.

@@             Coverage Diff              @@
##               main    #1663      +/-   ##
============================================
- Coverage     69.93%   69.92%   -0.02%     
  Complexity     2766     2766              
============================================
  Files          1052     1052              
  Lines         92427    92464      +37     
  Branches       1790     1790              
============================================
+ Hits          64638    64652      +14     
- Misses        26898    26921      +23     
  Partials        891      891              
Flag Coverage Δ
java 61.01% <ø> (ø)
rust 71.76% <39.47%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
src/common/src/types/interval.rs 57.14% <0.00%> (-4.93%) ⬇️
src/expr/src/expr/expr_binary_nonnull.rs 77.73% <ø> (ø)
src/expr/src/vector_op/arithmetic_op.rs 72.63% <0.00%> (-5.34%) ⬇️
src/frontend/src/expr/type_inference.rs 94.17% <100.00%> (+0.21%) ⬆️

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

Signed-off-by: TennyZhuang <zty0826@gmail.com>
Copy link
Contributor

@neverchanje neverchanje left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +275 to +288
build_binary_funcs(
&mut map,
&[E::Multiply],
&[T::Interval],
&[T::Int16, T::Int32, T::Int64],
T::Interval,
);
build_binary_funcs(
&mut map,
&[E::Multiply],
&[T::Int16, T::Int32, T::Int64],
&[T::Interval],
T::Interval,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to add build_cummulative_binary_ops to combine the two definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree! I can do it in next PR, there are many other types can use this.

Copy link
Contributor

@xiangjinwu xiangjinwu Apr 7, 2022

Choose a reason for hiding this comment

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

#1666/#1386

If we are going to support implicit cast, this table will be replaced completely (#1648), and repetition of cumulative ops needs to be handled in the new table.

If we decide to avoid implicit cast completely, there are a lot more improvements can be done in this builder fn.

Copy link
Contributor

@xiangjinwu xiangjinwu left a comment

Choose a reason for hiding this comment

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

lgtm. As mentioned in #1666, pg actually support int2/int4/int8/numeric/float4/float8. It can be supported either with generics or implicit cast. For now we can keep this implementation.

@TennyZhuang TennyZhuang merged commit fbaad8f into main Apr 7, 2022
@TennyZhuang TennyZhuang deleted the feat/interval-mul-int branch April 7, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants