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(expression): Implement IS [NOT] DISTINCT FROM in backend #2676

Merged
merged 8 commits into from
May 20, 2022

Conversation

Graphcalibur
Copy link
Contributor

@Graphcalibur Graphcalibur commented May 19, 2022

What's changed and what's your intention?

  • Move implementation of IS [NOT] DISTINCT FROM from frontend to backend to avoid repeated evaluation of expressions (see feat(expr): Implement IS [NOT] DISTINCT FROM expression #2582)
    • For IS NOT DISTINCT FROM, the frontend just binds a NOT to the bound IS DISTINCT FROM expression
  • Add unit and integration tests for IS [NOT] DISTINCT FROM

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)

Fixes #2684

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #2676 (be3b69f) into main (6652048) will increase coverage by 0.19%.
The diff coverage is 69.87%.

@@            Coverage Diff             @@
##             main    #2676      +/-   ##
==========================================
+ Coverage   72.14%   72.34%   +0.19%     
==========================================
  Files         681      685       +4     
  Lines       88502    89062     +560     
==========================================
+ Hits        63849    64430     +581     
+ Misses      24653    24632      -21     
Flag Coverage Δ
rust 72.34% <69.87%> (+0.19%) ⬆️

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

Impacted Files Coverage Δ
src/expr/src/expr/expr_binary_nonnull.rs 83.57% <ø> (ø)
src/expr/src/expr/expr_binary_nullable.rs 0.00% <0.00%> (ø)
src/expr/src/expr/mod.rs 48.83% <ø> (+1.21%) ⬆️
src/expr/src/expr/template.rs 60.24% <ø> (ø)
src/expr/src/vector_op/cmp.rs 55.03% <50.00%> (-0.52%) ⬇️
src/expr/src/vector_op/tests.rs 100.00% <100.00%> (ø)
src/frontend/src/binder/expr/mod.rs 84.28% <100.00%> (+11.07%) ⬆️
src/frontend/src/expr/type_inference.rs 98.85% <100.00%> (+0.94%) ⬆️
src/common/src/config.rs 43.43% <0.00%> (-37.19%) ⬇️
...c/frontend/src/optimizer/plan_node/batch_update.rs 51.11% <0.00%> (-23.09%) ⬇️
... and 62 more

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

Copy link
Contributor

@lmatz lmatz left a comment

Choose a reason for hiding this comment

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

LGTM!

@Graphcalibur Graphcalibur requested a review from xiangjinwu May 19, 2022 12:16
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

Some comments out of the scope of this PR:
Historically, when comparing decimal against f32, we though they should TryInto decimal or f64. But in PostgreSQL the common type for them is f32.

This affects existing comparisons including eq, ne, lt, etc as well. The macro is using f64 and some unit tests are using decimal. This can be fixed in a separate PR.

Comment on lines 643 to 646
/// `for_all_cmp_types` helps in matching and casting types when building comparison expressions
/// such as <= or IS DISTINCT FROM.
#[macro_export]
macro_rules! for_all_cmp_variants {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this macro in crate expr rather than in crate common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Would /expr/src/expr/template.rs be a suitable place for it?

@lmatz lmatz changed the title Implement IS [NOT] DISTINCT FROM in backend feat(expression): Implement IS [NOT] DISTINCT FROM in backend May 20, 2022
@Graphcalibur Graphcalibur requested a review from xiangjinwu May 20, 2022 07:11
@Graphcalibur Graphcalibur merged commit 36a294c into main May 20, 2022
@Graphcalibur Graphcalibur deleted the steven/distinct_from_backend branch May 20, 2022 07:53
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.

Implement IS (NOT) DISTINCT FROM` functions
3 participants