-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix the comparisions involving EMPTY. #5433
Conversation
3aa6665
to
c74acf8
Compare
215c016
to
44c497f
Compare
c09cb75
to
df2b780
Compare
…n false; otherwise, return null.
6fe448c
to
c847ed0
Compare
src/common/datatypes/Value.cpp
Outdated
if (empty() || v.empty()) { | ||
if (!empty() || !v.empty()) { | ||
return false; | ||
} else { | ||
return Value::kNullValue; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This writing style is a bit strange. Could you replace it with the following?
if (empty() && v.empty()) return Value::kNullValue;
if (empty() || v.empty()) return false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would leave two extra conditions for normal non-empty cases. Changed to another form instead.
@@ -20,6 +20,9 @@ folly::Future<Status> LoopExecutor::execute() { | |||
QueryExpressionContext ctx(ectx_); | |||
|
|||
auto value = expr->eval(ctx); | |||
if (value.isNull()) { | |||
value = Value(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that u should assign false to the value. maybe you need not to check the result of value, just delete the following DCHECK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't pass the tck. Reverted for now.
It is an expression containing at least one EMPTY
here. This place is expecting a TRUE value, if nothing more is introduced to change this part.
vesoft-inc/nebula-docs-cn#2716 |
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
Fix #5432
Description:
EMPTY == EMPTY
shall not return true.How do you solve it?
Tried to return null in all cases if an EMPTY value is involved in comparisions. But some cases have relied on it to return FALSE. Thus, I applied the above truth table for
==
and<
.Special notes for your reviewer, ex. impact of this fix, design document, etc:
I think we currently haven't explained any truth table involving EMTPY or EMTPY-like values in our documents. Right? Not sure.
Simlar to NULL (https://docs.nebula-graph.com.cn/3.4.0/3.ngql-guide/3.data-types/5.null/), we may need to explain the behaviours involving EMTPY or EMTPY-like values.
For example, queries like
would return nothing, as the property
nnnnnnnnnnnnnn
does not exist (EMPTY) andEMTPY == EMPTY
= NULL.Checklist:
Tests:
Affects:
Release notes:
Please confirm whether to be reflected in release notes and how to describe: