fix wrong expr evaluation on bool-type property in with

[xtcyclist]

What type of PR is this?

• bug
• feature
• enhancement

What problem(s) does this PR solve?

Issue(s) number:

Close https://github.com/vesoft-inc/nebula-ent/issues/1574

Description:

An e.non-exist-prop == true expr is rewritten to e.non-exist-prop without checking whether e.non-exist-prop exists. This is fine by itself.

And, while the filter operator evaluates the e.non-exist-prop condition, it finds it to be of the Value::kNullUnknownProp type. This causes unexpected error messages that terminte the query.

After discussion, we've decided that e.non-exist-prop == true shall return a plain null value.

How do you solve it?

e.non-exist-prop == true returns plain null.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

It may be the case that all nulls of the unkown_prop type shall be changed to plain ones. But I want to keep the impact of a bug fix to its minimal level. So I only made this change for edges, excluding tags' properties. This may cause inconsistent nulls in the results, which I think is probabaly a minor issue.

Checklist:

Tests:

• Unit test(positive and negative cases)
• Function test
• Performance test
• TCK

Affects:

• Documentation affected (Please add the label if documentation needs to be modified.)
• Incompatibility (If it breaks the compatibility, please describe it and add the label.）
• If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
• Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

[codecov-commenter]

Codecov Report

Base: 84.56% // Head: 84.63% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (9d940c7) compared to base (63a63ac).
Patch coverage: 84.95% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4846      +/-   ##
==========================================
+ Coverage   84.56%   84.63%   +0.07%     
==========================================
  Files        1358     1358              
  Lines      136387   136399      +12     
==========================================
+ Hits       115331   115446     +115     
+ Misses      21056    20953     -103     

Impacted Files	Coverage Δ
src/kvstore/Listener.h	39.13% <ø> (ø)
src/kvstore/Part.h	100.00% <ø> (ø)
src/kvstore/raftex/RaftPart.h	100.00% <ø> (ø)
src/kvstore/wal/FileBasedWal.h	100.00% <ø> (ø)
src/graph/visitor/VidExtractVisitor.cpp	48.83% <50.00%> (-0.15%)	⬇️
src/kvstore/wal/test/FileBasedWalTest.cpp	88.88% <52.38%> (ø)
src/kvstore/Listener.cpp	79.39% <67.34%> (+0.86%)	⬆️
src/storage/test/GetPropTest.cpp	94.80% <88.46%> (-0.54%)	⬇️
src/graph/util/ExpressionUtils.cpp	94.76% <93.75%> (-0.11%)	⬇️
src/graph/util/ParserUtil.cpp	94.01% <93.75%> (-0.16%)	⬇️
... and 61 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Shylock-Hg]

I think NullUnkownProp is also treated as null in filter.

[xtcyclist]

I think NullUnkownProp is also treated as null in filter.

I think NullUnkownProp is also treated as null in filter.

It is null, but it is bad null which would trigger a query-terminating error message.

  bool isBadNull() const {
    if (!isNull()) {
      return false;
    }
    auto& null = value_.nVal;
    return null == NullType::NaN || null == NullType::BAD_DATA || null == NullType::BAD_TYPE ||
           null == NullType::ERR_OVERFLOW || null == NullType::UNKNOWN_PROP ||
           null == NullType::DIV_BY_ZERO || null == NullType::OUT_OF_RANGE;
  }

We've discussed about the design of using Value to store some abnormality. It seems to be error-prone, but we'd better keep it as is for now to minimize changes.

[yixinglu]

LGTM