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.

Sign up for GitHub

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

Jump to bottom

Fix left range index #1498

Merged
merged 4 commits into from
Aug 11, 2021
Merged

Fix left range index #1498

merged 4 commits into from
Aug 11, 2021

Conversation

jadepeng
Copy link
Contributor

@jadepeng jadepeng commented Jun 15, 2021 •
edited by imbajin
Loading

fixed #1495

javeme
javeme reviewed Jun 15, 2021
View reviewed changes
hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexCoreTest.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/config/CoreOptions.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 16, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 17, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated
@@ -444,7 +444,7 @@ public boolean test(HugeElement element) {
return false;
}

if (hasErrorMatchedRangeIndex(element, cond)) {
if (!validRangeIndex(element, cond)) {
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 the same issue with search index and union index?
can remove the empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我的理解search index在搜索的时候有过滤，应该还好
union index 需要验证下

Copy link
Contributor

Choose a reason for hiding this comment

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

union index 看起来是需要解决这个问题的。

Copy link
Contributor

Choose a reason for hiding this comment

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

this.validRangeIndex()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

union index 看起来是需要解决这个问题的。

验证，union index 可以正确过滤，见新增的测试用例

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 17, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 24, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated
@@ -444,7 +444,7 @@ public boolean test(HugeElement element) {
return false;
}

if (hasErrorMatchedRangeIndex(element, cond)) {
if (!validRangeIndex(element, cond)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

union index 看起来是需要解决这个问题的。

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated
@@ -444,7 +444,7 @@ public boolean test(HugeElement element) {
return false;
}

if (hasErrorMatchedRangeIndex(element, cond)) {
if (!validRangeIndex(element, cond)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this.validRangeIndex()

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated

public static class Element2IndexValueMap {

Id propertyId;
Copy link
Contributor

Choose a reason for hiding this comment

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

set private final

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
@jadepeng jadepeng force-pushed the fix-outdated-range-index branch from e224874 to fc72dd7 Compare June 25, 2021 01:33
@javeme
Copy link
Contributor

javeme commented Jun 25, 2021

there are too many logs like:

2021-06-25 02:51:10 [main] [INFO] c.b.h.b.Transaction - Remove left index: v[678:soft1], query: `Query * from VERTEX where [LABEL == 678, 1852 > 0]`

javeme
javeme reviewed Jun 28, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated
@@ -613,6 +613,7 @@ private IdHolder doIndexQueryBatch(IndexLabel indexLabel,
this.removeExpiredIndexIfNeeded(index, query.showExpired());
ids.addAll(index.elementIds());
Query.checkForceCapacity(ids.size());
recordIndexValue(query, index, indexLabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.recordIndexValue

hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 29, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/query/ConditionQuery.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphIndexTransaction.java Outdated Show resolved Hide resolved
hugegraph-test/src/main/java/com/baidu/hugegraph/core/VertexCoreTest.java Outdated Show resolved Hide resolved
@imbajin
Copy link
Member

imbajin commented Jun 30, 2021 •
edited
Loading

@jadepeng could u rebase the master branch to fix the code coverage CI problem? (refer #1529)

@jadepeng jadepeng force-pushed the fix-outdated-range-index branch from f6fb0f0 to 1a503fe Compare June 30, 2021 08:29
@jadepeng
Copy link
Contributor Author

@jadepeng could u rebase the master branch to fix the code coverage CI problem? (refer #1529)

already rebase master

@codecov
Copy link

codecov bot commented Jun 30, 2021 •
edited
Loading

Codecov Report

Merging #1498 (ff4f301) into master (ef37b3a) will increase coverage by 4.79%.
The diff coverage is 86.39%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1498      +/-   ##
============================================
+ Coverage     58.07%   62.86%   +4.79%     
- Complexity     6244     6680     +436     
============================================
  Files           417      417              
  Lines         34293    34438     +145     
  Branches       4735     4764      +29     
============================================
+ Hits          19914    21650    +1736     
+ Misses        12310    10549    -1761     
- Partials       2069     2239     +170
Impacted Files Coverage Δ
...in/java/com/baidu/hugegraph/schema/IndexLabel.java 77.52% <55.55%> (-2.48%) ⬇️
...m/baidu/hugegraph/backend/tx/GraphTransaction.java 79.87% <73.68%> (+0.10%) ⬆️
...du/hugegraph/backend/tx/GraphIndexTransaction.java 83.97% <87.23%> (+1.93%) ⬆️
.../baidu/hugegraph/backend/query/ConditionQuery.java 86.11% <91.30%> (+1.60%) ⬆️
...n/java/com/baidu/hugegraph/config/CoreOptions.java 99.47% <100.00%> (+0.01%) ⬆️
...om/baidu/hugegraph/api/filter/ExceptionFilter.java 60.20% <0.00%> (-2.05%) ⬇️
...ain/java/com/baidu/hugegraph/task/TaskManager.java 69.06% <0.00%> (-1.44%) ⬇️
.../com/baidu/hugegraph/auth/StandardAuthManager.java 92.81% <0.00%> (-0.26%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef37b3a...ff4f301. Read the comment docs.

@jadepeng jadepeng force-pushed the fix-outdated-range-index branch 2 times, most recently from 0cd62de to 30ea45d Compare July 2, 2021 10:14
@jadepeng jadepeng force-pushed the fix-outdated-range-index branch from 29bb384 to c01c767 Compare July 7, 2021 06:55
@jadepeng jadepeng force-pushed the fix-outdated-range-index branch from bf512d7 to 13926c4 Compare July 23, 2021 05:00
javeme
javeme previously approved these changes Jul 28, 2021
View reviewed changes
hugegraph-core/src/main/java/com/baidu/hugegraph/backend/tx/GraphTransaction.java
@@ -1620,6 +1608,23 @@ private void lockForUpdateProperty(SchemaLabel schemaLabel,
}
}

private void removeLeftIndexIfNeeded(Map<Id, HugeVertex> vertices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can mock and test it? use Whitebox to set removeLeftIndexOnOverwrite=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add test, but seems not work

zhoney
zhoney previously approved these changes Jul 29, 2021
View reviewed changes
@javeme
Copy link
Contributor

javeme commented Aug 6, 2021

failure test of cassandra backend:

Error:  Tests run: 713, Failures: 1, Errors: 0, Skipped: 21, Time elapsed: 500.427 s <<< FAILURE! - in com.baidu.hugegraph.core.CoreTestSuite
Error:  testRemoveLeftRangeIndex(com.baidu.hugegraph.core.VertexCoreTest)  Time elapsed: 2.742 s  <<< FAILURE!
java.lang.AssertionError: expected:<10> but was:<6>
	at com.baidu.hugegraph.core.VertexCoreTest.testRemoveLeftRangeIndex(VertexCoreTest.java:536)

2021-07-28 11:37:05 [hugegraph-shutdown] [INFO] c.b.h.HugeGraph - HugeGraph is shutting down
Error: -28 11:37:05 [hugegraph-shutdown] [ERROR] c.b.h.HugeGraph - Error while shutdown
java.lang.AssertionError: 2
	at com.baidu.hugegraph.task.TaskManager.shutdown(TaskManager.java:180) ~[classes/:?]
	at com.baidu.hugegraph.HugeFactory.shutdown(HugeFactory.java:140) ~[classes/:?]
	at com.baidu.hugegraph.HugeFactory.lambda$static$0(HugeFactory.java:52) ~[classes/:?]
	at java.lang.Thread.run(Thread.java:748) [?:1.8.0_292]
Error: Exception in thread "hugegraph-shutdown" com.baidu.hugegraph.HugeException: Failed to shutdown
	at com.baidu.hugegraph.HugeFactory.shutdown(HugeFactory.java:144)
	at com.baidu.hugegraph.HugeFactory.lambda$static$0(HugeFactory.java:52)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.AssertionError: 2
	at com.baidu.hugegraph.task.TaskManager.shutdown(TaskManager.java:180)
	at com.baidu.hugegraph.HugeFactory.shutdown(HugeFactory.java:140)
	... 2 more
[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    VertexCoreTest.testRemoveLeftRangeIndex:536 expected:<10> but was:<6>
[INFO] 
Error:  Tests run: 713, Failures: 1, Errors: 0, Skipped: 21

@jadepeng
Copy link
Contributor Author

failure test of cassandra backend:

Error:  Tests run: 713, Failures: 1, Errors: 0, Skipped: 21, Time elapsed: 500.427 s <<< FAILURE! - in com.baidu.hugegraph.core.CoreTestSuite
Error:  testRemoveLeftRangeIndex(com.baidu.hugegraph.core.VertexCoreTest)  Time elapsed: 2.742 s  <<< FAILURE!
java.lang.AssertionError: expected:<10> but was:<6>
	at com.baidu.hugegraph.core.VertexCoreTest.testRemoveLeftRangeIndex(VertexCoreTest.java:536)

2021-07-28 11:37:05 [hugegraph-shutdown] [INFO] c.b.h.HugeGraph - HugeGraph is shutting down
Error: -28 11:37:05 [hugegraph-shutdown] [ERROR] c.b.h.HugeGraph - Error while shutdown
java.lang.AssertionError: 2
	at com.baidu.hugegraph.task.TaskManager.shutdown(TaskManager.java:180) ~[classes/:?]
	at com.baidu.hugegraph.HugeFactory.shutdown(HugeFactory.java:140) ~[classes/:?]
	at com.baidu.hugegraph.HugeFactory.lambda$static$0(HugeFactory.java:52) ~[classes/:?]
	at java.lang.Thread.run(Thread.java:748) [?:1.8.0_292]
Error: Exception in thread "hugegraph-shutdown" com.baidu.hugegraph.HugeException: Failed to shutdown
	at com.baidu.hugegraph.HugeFactory.shutdown(HugeFactory.java:144)
	at com.baidu.hugegraph.HugeFactory.lambda$static$0(HugeFactory.java:52)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.AssertionError: 2
	at com.baidu.hugegraph.task.TaskManager.shutdown(TaskManager.java:180)
	at com.baidu.hugegraph.HugeFactory.shutdown(HugeFactory.java:140)
	... 2 more
[INFO] 
[INFO] Results:
[INFO] 
Error:  Failures: 
Error:    VertexCoreTest.testRemoveLeftRangeIndex:536 expected:<10> but was:<6>
[INFO] 
Error:  Tests run: 713, Failures: 1, Errors: 0, Skipped: 21

fixed

@jadepeng jadepeng closed this Aug 11, 2021
@jadepeng jadepeng reopened this Aug 11, 2021
@zhoney zhoney merged commit cba3c23 into apache:master Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhoney zhoney zhoney approved these changes

@javeme javeme javeme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] range index 残留索引导致统计不准确
4 participants
@jadepeng @javeme @imbajin @zhoney