Repository navigation
fix(store): stop ProjectionStage clearing dedup keys in place - #3408
Conversation
MapLimitIterator keeps each emitted RocksDBSession.BackendColumn as a key in its dedup set and passes the same object downstream. On an id-only scan with every property removed, ProjectionStage then set column.value to an empty array in place, so the stored key no longer equalled a later byte-equal row and the copy was emitted again: 200 rows for 100 keys read twice. - replace the projected column instead of clearing value on the shared object - keep the dedup key MapLimitIterator holds unchanged - cover both with ProjectionStageTest in ServerSuiteTest Fixes apache#3407
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3408 +/- ##
============================================
- Coverage 55.93% 55.69% -0.25%
- Complexity 2277 2423 +146
============================================
Files 774 775 +1
Lines 68534 69521 +987
Branches 9333 9618 +285
============================================
+ Hits 38334 38717 +383
- Misses 26634 27192 +558
- Partials 3566 3612 +46
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: ProjectionStage.handle() replaces the result column through setColumn(BackendColumn.of(name, new byte[0])), so the key MapLimitIterator holds keeps its value and its hashCode, matching the replace pattern OlapStage uses. ProjectionStageTest is registered in ServerSuiteTest, and the pd_store / store job runs it at this head (182 tests, 0 failures).
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: Replacing the projected column preserves the name/value identity retained by MapLimitIterator while keeping the serialized id-only result unchanged. The regression tests cover input preservation and interleaved projection of byte-equal duplicate rows. Evidence: Traced BackendColumn equality/hashCode, PipelineResult and AggregativeQueryObserver serialization; exact-head Store CI completed the registered server suite with 182 tests and zero failures, and HStore/cluster checks passed. The project coverage check is a non-blocking quality signal. Score: 8.7/10.
…#3410) The existing tests drive ProjectionStage by hand, so they would miss a change in how buildPlan wires the projection for a null_property request. The new case checks that buildPlan returns a projection-only plan for that request, feeds MapLimitIterator output through it, and checks each key comes out once with an empty value. It fails with "key 0 emitted twice" when the ProjectionStage fix from #3408 is reverted.
Purpose of the PR
Follow-up to #3404, from @bitflicker64's review there.
MapLimitIteratorkeeps each emittedRocksDBSession.BackendColumnin its dedup set and passes the same object downstream. On an id-onlyPRIMARY_SCAN(null_property = true, no property list, condition, order-by or aggregation) withdedup_option = LIMIT_DEDUP,ProjectionStage.handle()setcolumn.value = new byte[0]on that object. The key stored in the set changed after insertion, so it no longer equalled a later byte-equal row and the copy was emitted again. @bitflicker64's probe got 200 rows for 100 keys read twice; onmasterthe new test fails withkey 0 emitted twice.Main Changes
ProjectionStage.handle(): in theBACKEND_COLUMNbranch withremoveAllProperty, replace the result's column withBackendColumn.of(column.name, new byte[0])instead of clearingvalueon the shared object.ProjectionStageTestinhg-store-test, registered inServerSuiteTest:MapLimitIteratorfollowed by the projection on each emitted row returns each of 100 keys once.Only a gRPC client that sets
dedup_optionreaches this path today, sinceStoreQueryParamfixes it toNONE.Verifying these changes
mvn test -pl hugegraph-store/hg-store-test -am -P store-server-test -Dtest=ProjectionStageTest -Dsurefire.failIfNoSpecifiedTests=false: both tests fail onmaster(a key is emitted twice; the input column's value is cleared) and pass with this change.mvn test -pl hugegraph-store/hg-store-test -am -P store-server-test: 182 tests, 0 failures.mvn test -pl hugegraph-store/hg-store-test -am -P store-core-test: 141 tests, 0 failures (1 skipped,IpUtilTest, an environment assumption).mvn editorconfig:check -pl hugegraph-store/hg-store-test -ampasses.Does this PR potentially affect the following parts?
Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.Documentation files in this PR or paired hugegraph-doc PR: