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

Merged
merged 9 commits into from
May 17, 2022

Conversation

Graphcalibur
Copy link
Contributor

@Graphcalibur Graphcalibur commented May 16, 2022

What's changed and what's your intention?

  • This PR implements the IS [NOT] DISTINCT FROM expression.
  • This was done by using the existing implementations for expressions such as IsNull, IsNotNull, Equals, and NotEquals to build a binding with the same truth value as 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)

#2513

@CLAassistant
Copy link

CLAassistant commented May 16, 2022

CLA assistant check
All committers have signed the CLA.

@Graphcalibur Graphcalibur requested a review from lmatz May 16, 2022 16:49
@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #2582 (8c1f9cf) into main (8d0d69b) will increase coverage by 0.17%.
The diff coverage is 30.76%.

@@            Coverage Diff             @@
##             main    #2582      +/-   ##
==========================================
+ Coverage   72.19%   72.37%   +0.17%     
==========================================
  Files         674      678       +4     
  Lines       87853    88038     +185     
==========================================
+ Hits        63422    63714     +292     
+ Misses      24431    24324     -107     
Flag Coverage Δ
rust 72.37% <30.76%> (+0.17%) ⬆️

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

Impacted Files Coverage Δ
src/frontend/src/binder/expr/mod.rs 73.20% <30.76%> (-12.28%) ⬇️
src/bench/ss_bench/utils/my_stats.rs 0.00% <0.00%> (-72.42%) ⬇️
src/batch/src/executor2/generate_series.rs 0.00% <0.00%> (-68.86%) ⬇️
src/batch/src/executor2/generate_time_series.rs 66.66% <0.00%> (-5.75%) ⬇️
src/common/src/types/chrono_wrapper.rs 72.91% <0.00%> (-1.35%) ⬇️
src/storage/src/monitor/state_store_metrics.rs 98.17% <0.00%> (-1.34%) ⬇️
src/storage/src/hummock/sstable_store.rs 65.11% <0.00%> (-0.81%) ⬇️
src/frontend/src/scheduler/execution/stage.rs 45.88% <0.00%> (-0.61%) ⬇️
src/expr/src/expr/build_expr_from_prost.rs 69.50% <0.00%> (-0.22%) ⬇️
... and 45 more

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

@lmatz lmatz requested a review from xiangjinwu May 17, 2022 02:37
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

FunctionCall::new_unchecked(
ExprType::Or,
vec![
FunctionCall::new_unchecked(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any preference for new_unchecked over new or the other way around? @xiangjinwu

Copy link
Contributor

Choose a reason for hiding this comment

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

As the name and doc suggested, new_unchecked bypasses type checking and should only be used for a good reason.
Here the innermost expressions (IsNull, IsNotNull, Equal, NotEqual) are using new correctly, and outer expressions (And, Or) are bypassing the checks because both operands are from validated inner expressions. This is acceptable, but maybe the performance gain is negligible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw each clone of expr means repeated evaluation in the backend. It may be better to implement these 2 operators in backend directly rather than rewriting to such long expressions in the frontend.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, let this PR add some tests first to make sure the semantics are all correct.

Then another PR implements the operators in the backend and removes the rewriting.

@Graphcalibur
Copy link
Contributor Author

The expression binding is fixed so that it can now only evaluate to either true or false (instead of unknown). E2E tests were also added.

@Graphcalibur Graphcalibur requested a review from lmatz May 17, 2022 13:32
@lmatz lmatz merged commit 93ac770 into main May 17, 2022
@lmatz lmatz deleted the steven/distinct_from branch May 17, 2022 15:11
@xiangjinwu xiangjinwu added the user-facing-changes Contains changes that are visible to users label May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants