[PR] fix testGroupByLevel2Series2Devices3Regions [iotdb]

["kabo87777 (via GitHub)" <gi...@apache.org>]

kabo87777 opened a new pull request, #16613:
URL: https://github.com/apache/iotdb/pull/16613

   # Fix Non-Deterministic Behavior in AggregationDistributionTest.testGroupByLevel2Series2Devices3Regions
   
   ## Problem
   The test `testGroupByLevel2Series2Devices3Regions` was failing non-deterministically under NonDex with a 60% failure rate (3 out of 5 runs) due to order-dependent fragment access and assumptions about node positions in the plan tree.
   
   ## Way to Reproduce
   
   ```bash
   cd iotdb-core/datanode
   mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex \
     -Dtest=AggregationDistributionTest#testGroupByLevel2Series2Devices3Regions \
     -DnondexRuns=5
   
   # Expected: Test fails with certain NonDex seeds
   # Failure: AssertionError or ClassCastException when accessing nodes at fixed positions
   ```
   
   ## Root Cause
   The test made multiple order-dependent assumptions:
   1. **Fixed fragment indices**: Accessed fragments by hardcoded positions (`get(0)`, `get(2)`)
   2. **Fixed tree positions**: Assumed `GroupByLevelNode` is always at specific child positions
   3. **Rigid structure checks**: Used `instanceof` checks at fixed depth levels
   
   ```java
   // Assumed fragment 0 has nested GroupByLevelNode at specific position
   assertTrue(
       fragmentInstances.get(0)
           .getFragment().getPlanNodeTree()
           .getChildren().get(0)
           .getChildren().get(0)
       instanceof GroupByLevelNode);
   
   // Assumed fragment 2 has GroupByLevelNode at first child
   assertTrue(
       fragmentInstances.get(2).getFragment().getPlanNodeTree()
           .getChildren().get(0)
       instanceof GroupByLevelNode);
   
   // Assumed descriptor patterns match fragment order
   verifyGroupByLevelDescriptor(expectedDescriptorValue,
       fragmentInstances.get(0)...);
   verifyGroupByLevelDescriptor(expectedDescriptorValue2,
       fragmentInstances.get(2)...);
   ```
   
   When NonDex shuffled collection iteration order, fragments appeared in different sequences and with varying tree structures, causing failures despite the query plan being semantically correct.
   
   ## Solution
   Made the test **order-independent** by searching all fragments for expected patterns instead of relying on fixed positions:
   
   ### 1. Added Helper Method for Tree Search
   ```java
   private <T extends PlanNode> T findFirstNodeOfType(PlanNode root, Class<T> nodeType) {
     if (root == null) return null;
     if (nodeType.isInstance(root)) return (T) root;
     for (PlanNode child : root.getChildren()) {
       T result = findFirstNodeOfType(child, nodeType);
       if (result != null) return result;
     }
     return null;
   }
   ```
   
   ### 2. Changed from Position-Based to Pattern-Based Verification
   
   **Before (Order-Dependent):**
   ```java
   // Hardcoded positions
   verifyGroupByLevelDescriptor(expectedDescriptorValue,
       (GroupByLevelNode) fragmentInstances.get(0)
           .getFragment().getPlanNodeTree().getChildren().get(0));
   verifyGroupByLevelDescriptor(expectedDescriptorValue2,
       (GroupByLevelNode) fragmentInstances.get(2)
           .getFragment().getPlanNodeTree().getChildren().get(0));
   ```
   
   **After (Order-Independent):**
   ```java
   // Define expected descriptor patterns
   Map<String, List<String>> expectedDescriptorValue1 = new HashMap<>();
   expectedDescriptorValue1.put(groupedPathS1, Arrays.asList(groupedPathS1, d2s1Path));
   expectedDescriptorValue1.put(groupedPathS2, Collections.singletonList(groupedPathS2));
   
   Map<String, List<String>> expectedDescriptorValue2 = new HashMap<>();
   expectedDescriptorValue2.put(groupedPathS1, Collections.singletonList(d1s1Path));
   expectedDescriptorValue2.put(groupedPathS2, Collections.singletonList(d1s2Path));
   
   // Search all fragments for matching patterns
   boolean foundPattern1 = false;
   boolean foundPattern2 = false;
   int groupByLevelNodeCount = 0;
   
   for (FragmentInstance instance : fragmentInstances) {
     PlanNode root = instance.getFragment().getPlanNodeTree();
     if (countNodesOfType(root, GroupByLevelNode.class) > 0) {
       groupByLevelNodeCount++;
       // Find GroupByLevelNode at any depth
       GroupByLevelNode groupByLevel = findFirstNodeOfType(root, GroupByLevelNode.class);
       if (groupByLevel != null) {
         try {
           verifyGroupByLevelDescriptor(expectedDescriptorValue1, groupByLevel);
           foundPattern1 = true;
         } catch (AssertionError e) {
           try {
             verifyGroupByLevelDescriptor(expectedDescriptorValue2, groupByLevel);
             foundPattern2 = true;
           } catch (AssertionError e2) {
             // This fragment doesn't match any expected pattern
           }
         }
       }
     }
   }
   
   // Verify both patterns exist somewhere in the plan
   assertTrue("Expected at least 2 GroupByLevelNode instances", groupByLevelNodeCount >= 2);
   assertTrue("Expected to find fragment matching pattern 1", foundPattern1);
   assertTrue("Expected to find fragment matching pattern 2", foundPattern2);
   ```
   
   ### Key Improvements:
   1. **Recursive search**: Uses `findFirstNodeOfType()` to locate `GroupByLevelNode` at any depth
   2. **Pattern matching**: Verifies descriptor patterns instead of fragment positions
   3. **Flexible counting**: Ensures minimum required nodes exist without assuming exact count
   4. **Try-catch pattern matching**: Attempts to match each fragment against expected patterns
   
   ## Verification
   Tested with 50 NonDex runs - **0 failures** (100% success rate):
   ```bash
   mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex \
     -Dtest=AggregationDistributionTest#testGroupByLevel2Series2Devices3Regions \
     -DnondexRuns=50
   # Result: 0 failures
   ```
   
   ## Key Changed Classes
   - **AggregationDistributionTest**: 
     - Modified `testGroupByLevel2Series2Devices3Regions` to use order-independent verification
     - Added `findFirstNodeOfType()` helper method for recursive node search at any depth
     - Leverages existing `countNodesOfType()` helper method


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org