Skip to content

test(store): cover the id-only projection through QueryUtil.buildPlan - #3410

Merged
imbajin merged 1 commit into
apache:masterfrom
vaidikgarg-bit:test/projection-plan-dedup
Oct 27, 2026
Merged

imbajin merged 1 commit into
apache:masterfrom
vaidikgarg-bit:test/projection-plan-dedup

Conversation

@vaidikgarg-bit

Copy link
Copy Markdown
Contributor

Purpose of the PR

The tests in #3408 call ProjectionStage directly, so they would still pass if QueryUtil.buildPlan stopped adding the projection for a null_property request or initialized it without removeAllProperty. This adds one case that runs MapLimitIterator output through the plan the server builds, checking that id-only rows come back with an empty value and that the limit dedup still drops repeated keys on that path.

Main Changes

  • ProjectionStageTest.testLimitDedupStillDropsCopiesThroughIdOnlyPlan: builds the plan with QueryUtil.buildPlan for a request with sample_factor = 1.0 and null_property = true, asserts it is QueryPlan{stages=[PROJECTION_STAGE]}, feeds MapLimitIterator output through plan.execute(...) one row at a time, and checks that each of 100 keys comes out once with an empty value.

sample_factor is set because its proto default, 0.0, builds a stop-only plan.

No production code changes.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • mvn test -pl hugegraph-store/hg-store-test -am -P store-server-test -Dtest=ProjectionStageTest -Dsurefire.failIfNoSpecifiedTests=false: 3 tests pass. With ProjectionStage.java reverted to its state before fix(store): stop ProjectionStage clearing dedup keys in place #3408, all three fail: the new test and testLimitDedupStillDropsCopiesAfterProjection with key 0 emitted twice, and testRemoveAllPropertyLeavesInputColumnUnchanged on its assertSame. If buildPlan stops adding the projection, only the new test fails.
    • mvn test -pl hugegraph-store/hg-store-test -am -P store-server-test: 183 tests, 0 failures.
    • mvn editorconfig:check -pl hugegraph-store/hg-store-test -am passes.

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:

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 apache#3408 is reverted.
@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.28%. Comparing base (608cb1d) to head (0ecf770).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3410      +/-   ##
============================================
+ Coverage     57.27%   57.28%   +0.01%     
+ Complexity     6577     2395    -4182     
============================================
  Files           776      776              
  Lines         68881    68876       -5     
  Branches       9431     9431              
============================================
+ Hits          39449    39454       +5     
+ Misses        25804    25793      -11     
- Partials       3628     3629       +1     
Flag Coverage Δ *Carryforward flag
commons 96.37% <ø> (ø) Carriedforward from 608cb1d
hstore 36.11% <ø> (+0.02%) ⬆️
pd 52.17% <ø> (+<0.01%) ⬆️ Carriedforward from 608cb1d
server_hbase 42.18% <ø> (ø) Carriedforward from 608cb1d
server_memory 55.98% <ø> (ø) Carriedforward from 608cb1d
server_rocksdb 42.65% <ø> (ø) Carriedforward from 608cb1d
store 33.12% <ø> (-0.03%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking: no. Score: 9/10. Summary: The new case builds the plan through QueryUtil.buildPlan for an id-only request and fails if the projection is dropped, initialized without removeAllProperty, or mutates the column in place; ServerSuiteTest picks it up and the store-server-test CI run reports 183 tests passing.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: no. Summary: The regression test exercises the id-only QueryUtil.buildPlan path and detects missing projection, incorrect property removal and mutation that breaks limit deduplication. Evidence: Three independent static reviews traced the plan, iterator hash semantics and ServerSuiteTest inclusion; exact-revision Store and HStore CI passed. No local Maven run was repeated. Score: 8.8/10.

@imbajin
imbajin merged commit 9a8f066 into apache:master Oct 27, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants