2860: minor: Small refactor in CometExecRule to remove confusing code and fix more fallback reporting

[martin-augment]

2860: To review by AI

[coderabbitai]

Walkthrough

This pull request modifies the Comet operator conversion logic in CometExecRule.scala and updates corresponding test expectations. The code changes replace the convertToCometIfAllChildrenAreNative method and its calls with more granular checks. Specifically, BroadcastExchangeExec conversion now only occurs when all children are CometNativeExec, while the fallback operator handling logic is restructured to check native-child status before conversion attempts. The convertToComet method is updated to require non-empty children for protobuf plan consolidation. Multiple TPC-DS plan stability test files are updated to reflect these logic changes, removing "[COMET: not supported]" annotations and showing expanded operator hierarchies with acceleration metrics.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr-2860-2025-12-09-07-16-55

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2ab8e12 and 4bfea30.

📒 Files selected for processing (13)

• spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala (3 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q28.native_iceberg_compat/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q28/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88.native_iceberg_compat/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q28.native_iceberg_compat/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q28/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88.native_iceberg_compat/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q28.native_iceberg_compat/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q28/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88.native_iceberg_compat/extended.txt (1 hunks)
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88/extended.txt (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-11-11T15:01:48.203Z

Learnt from: martin-augment
Repo: martin-augment/datafusion-comet PR: 17
File: docs/source/contributor-guide/adding_a_new_operator.md:349-354
Timestamp: 2025-11-11T15:01:48.203Z
Learning: For Apache DataFusion Comet debugging documentation, the correct configuration keys are `spark.comet.explain.format=verbose` for verbose explain plans and `spark.comet.logFallbackReasons.enabled=true` for logging fallback reasons (not `spark.comet.explain.verbose` or `spark.comet.logFallbackReasons` without `.enabled`).

Applied to files:

• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88.native_iceberg_compat/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q28.native_iceberg_compat/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q28.native_iceberg_compat/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q28/extended.txt
• spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88.native_iceberg_compat/extended.txt

🪛 LanguageTool

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88/extended.txt

[typographical] ~1-~1: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: BroadcastNestedLoopJoin :- BroadcastNestedLoopJoin : :- Broadcast...

(TIRET)

---

[typographical] ~3-~3: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...dLoopJoin :- BroadcastNestedLoopJoin : :- BroadcastNestedLoopJoin : : :- Broadc...

(TIRET)

---

[typographical] ~4-~4: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...oin : :- BroadcastNestedLoopJoin : : :- BroadcastNestedLoopJoin : : : :- Bro...

(TIRET)

---

[typographical] ~5-~5: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... : :- BroadcastNestedLoopJoin : : : :- BroadcastNestedLoopJoin : : : : :- ...

(TIRET)

---

[typographical] ~6-~6: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... :- BroadcastNestedLoopJoin : : : : :- BroadcastNestedLoopJoin : : : : : ...

(TIRET)

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88/extended.txt

[typographical] ~1-~1: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: BroadcastNestedLoopJoin :- BroadcastNestedLoopJoin : :- Broadcast...

(TIRET)

---

[typographical] ~3-~3: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...dLoopJoin :- BroadcastNestedLoopJoin : :- BroadcastNestedLoopJoin : : :- Broadc...

(TIRET)

---

[typographical] ~4-~4: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...oin : :- BroadcastNestedLoopJoin : : :- BroadcastNestedLoopJoin : : : :- Bro...

(TIRET)

---

[typographical] ~5-~5: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... : :- BroadcastNestedLoopJoin : : : :- BroadcastNestedLoopJoin : : : : :- ...

(TIRET)

---

[typographical] ~6-~6: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... :- BroadcastNestedLoopJoin : : : : :- BroadcastNestedLoopJoin : : : : : ...

(TIRET)

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88.native_iceberg_compat/extended.txt

[typographical] ~1-~1: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: BroadcastNestedLoopJoin :- BroadcastNestedLoopJoin : :- Broadcast...

(TIRET)

---

[typographical] ~3-~3: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...dLoopJoin :- BroadcastNestedLoopJoin : :- BroadcastNestedLoopJoin : : :- Broadc...

(TIRET)

---

[typographical] ~4-~4: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...oin : :- BroadcastNestedLoopJoin : : :- BroadcastNestedLoopJoin : : : :- Bro...

(TIRET)

---

[typographical] ~5-~5: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... : :- BroadcastNestedLoopJoin : : : :- BroadcastNestedLoopJoin : : : : :- ...

(TIRET)

---

[typographical] ~6-~6: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... :- BroadcastNestedLoopJoin : : : : :- BroadcastNestedLoopJoin : : : : : ...

(TIRET)

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88/extended.txt

[typographical] ~1-~1: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: BroadcastNestedLoopJoin :- BroadcastNestedLoopJoin : :- Broadcast...

(TIRET)

---

[typographical] ~3-~3: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...dLoopJoin :- BroadcastNestedLoopJoin : :- BroadcastNestedLoopJoin : : :- Broadc...

(TIRET)

---

[typographical] ~4-~4: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...oin : :- BroadcastNestedLoopJoin : : :- BroadcastNestedLoopJoin : : : :- Bro...

(TIRET)

---

[typographical] ~5-~5: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... : :- BroadcastNestedLoopJoin : : : :- BroadcastNestedLoopJoin : : : : :- ...

(TIRET)

---

[typographical] ~6-~6: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... :- BroadcastNestedLoopJoin : : : : :- BroadcastNestedLoopJoin : : : : : ...

(TIRET)

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88.native_iceberg_compat/extended.txt

[typographical] ~1-~1: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: BroadcastNestedLoopJoin :- BroadcastNestedLoopJoin : :- Broadcast...

(TIRET)

---

[typographical] ~3-~3: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...dLoopJoin :- BroadcastNestedLoopJoin : :- BroadcastNestedLoopJoin : : :- Broadc...

(TIRET)

---

[typographical] ~4-~4: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...oin : :- BroadcastNestedLoopJoin : : :- BroadcastNestedLoopJoin : : : :- Bro...

(TIRET)

---

[typographical] ~5-~5: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... : :- BroadcastNestedLoopJoin : : : :- BroadcastNestedLoopJoin : : : : :- ...

(TIRET)

---

[typographical] ~6-~6: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... :- BroadcastNestedLoopJoin : : : : :- BroadcastNestedLoopJoin : : : : : ...

(TIRET)

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88.native_iceberg_compat/extended.txt

[typographical] ~1-~1: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: BroadcastNestedLoopJoin :- BroadcastNestedLoopJoin : :- Broadcast...

(TIRET)

---

[typographical] ~3-~3: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...dLoopJoin :- BroadcastNestedLoopJoin : :- BroadcastNestedLoopJoin : : :- Broadc...

(TIRET)

---

[typographical] ~4-~4: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ...oin : :- BroadcastNestedLoopJoin : : :- BroadcastNestedLoopJoin : : : :- Bro...

(TIRET)

---

[typographical] ~5-~5: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... : :- BroadcastNestedLoopJoin : : : :- BroadcastNestedLoopJoin : : : : :- ...

(TIRET)

---

[typographical] ~6-~6: Le trait d’union est employé sans espaces pour former des mots, alors que le tiret est encadré par des espaces et placé entre deux mots distincts.
Context: ... :- BroadcastNestedLoopJoin : : : : :- BroadcastNestedLoopJoin : : : : : ...

(TIRET)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Cursor Bugbot
• GitHub Check: claude-review

🔇 Additional comments (18)

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88.native_iceberg_compat/extended.txt (1)

1-216: Test expectations update aligns with improved fallback reporting.

The expanded execution plan correctly reflects the PR's improvements to operator conversion logic. The plan structure is sound: the root BroadcastNestedLoopJoin is appropriately marked as unsupported (line 7), while the nested subtrees show successful Comet operator conversions (CometHashAggregate, CometExchange, CometBroadcastHashJoin, etc.). The final summary (93% acceleration with 8 Spark-Comet transitions) demonstrates improved coverage.

The static analysis warnings about French hyphenation are false positives—they're analyzing the ASCII tree structure (:- , +- ) used in standard Spark explain plans, not actual French text.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q88/extended.txt (1)

1-216: Test expectation file updated correctly to reflect expanded Comet conversion.

This test resource file has been updated from showing "[COMET: BroadcastNestedLoopJoin is not supported]" to displaying the full expanded execution plan with Comet operator conversions. The change is consistent with the PR objective of improving fallback reporting by showing actual operator hierarchies instead of suppressing details.

The file structure shows:

• Lines 1–6: Spark-native BroadcastNestedLoopJoin operators (remain unconverted)
• Lines 8–214: Detailed Comet-accelerated subplan with proper operator hierarchy (CometColumnarToRow → CometHashAggregate → CometExchange → CometProject → CometBroadcastHashJoin → CometFilter → CometScan)
• Line 216: Summary metrics indicating 93% acceleration with 8 Spark-to-Comet transitions

The metrics (192/206 operators, 93% acceleration, 8 transitions) align with the tree structure, where multiple BroadcastExchange nodes serve as the transition boundaries between Spark and Comet regions.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88.native_iceberg_compat/extended.txt (1)

1-216: Test resource file format is correct and consistent with PR objectives.

This file contains expected Spark execution plan output and is properly structured. The nested BroadcastNestedLoopJoin tree (lines 1–6) with the "[COMET: BroadcastNestedLoopJoin is not supported]" annotation on line 7, followed by the detailed Comet execution plan below it (lines 8–214), demonstrates the improved fallback reporting logic described in the PR. The final acceleration metrics (line 216) showing "93%" operator acceleration and "8 transitions between Spark and Comet" are reasonable for a complex multi-way join query.

The consistent use of tree structure syntax (:- , : :- indentation patterns) is correct for Spark plan representation. The file comprehensively shows both unsupported Spark operators and the expanded Comet plan hierarchy for converted child operators, which aligns with the PR's goal to "fix more fallback reporting."

To ensure this updated test expectation is correct, please verify that:

1. The actual CometExecRule.scala changes correctly implement the conversion logic that produces this expanded plan with fallback reporting
2. The test file was regenerated from an actual query execution, not manually edited
3. The performance metrics align with your expectations for this TPC-DS query

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q28.native_iceberg_compat/extended.txt (1)

1-78: LGTM! Test expectations updated to reflect improved fallback reporting.

The plan now shows a cleaner operator hierarchy without redundant "[COMET: not supported]" annotations at the top level, while preserving detailed fallback reasons at the actual blocking operators (e.g., Line 5, Line 10). The final acceleration summary confirms the expected behavior.

spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala (6)

194-236: LGTM! Simplified AQE query stage conversion.

Replacing convertToCometIfAllChildrenAreNative with convertToComet for AQE query stages is correct since these operators wrap known Comet exchanges. The direct conversion call is clearer and eliminates unnecessary checking.

---

206-226: LGTM! More explicit BroadcastExchangeExec conversion logic.

The explicit check at Line 208 makes it clear that BroadcastExchangeExec is only converted to CometBroadcastExchangeExec when all children are CometNativeExec. This improves code readability and aligns with the PR's goal to remove confusing logic.

---

241-253: LGTM! Generic operator conversion logic correctly replaces removed helper.

The new logic at Lines 244-250 explicitly handles conversion when all children are native by looking up the appropriate handler from allExecs. The early return pattern makes the control flow clear and correctly replaces the removed convertToCometIfAllChildrenAreNative helper.

---

255-261: LGTM! Appropriate expansion of the exclusion list.

Adding ExecutedCommandExec and V2CommandExec to the exclusion list is correct since these command execution operators should never be replaced with Comet equivalents. This prevents misleading fallback messages.

---

262-275: LGTM! More precise fallback reporting.

The updated logic at Line 270 correctly identifies when to tag an operator as "not supported" by checking both:

1. All children are native (operator itself is the blocker)
2. No existing explain info (not already tagged)

This eliminates redundant messages and improves fallback reporting accuracy, directly addressing the PR objective.

---

460-477: LGTM! Essential defensive check prevents edge case.

The nonEmpty check at Line 464 is critical to prevent vacuous truth issues. Without it, forall returns true for empty sequences, which could incorrectly trigger protobuf plan consolidation for leaf operators without children. This is good defensive programming.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q28.native_iceberg_compat/extended.txt (1)

1-78: LGTM! Test expectations correctly updated.

The updated plan reflects the improved fallback reporting from the refactored CometExecRule. Top-level redundant annotations are removed while preserving detailed fallback reasons at actual blocking operators.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q28.native_iceberg_compat/extended.txt (1)

1-78: LGTM! Test expectations correctly updated.

Consistent with other plan updates in this PR, showing cleaner operator hierarchies and improved fallback messaging.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q28/extended.txt (1)

1-78: LGTM! Test expectations correctly updated.

The plan updates are consistent with the refactored conversion logic, showing expanded operator details without redundant top-level "not supported" messages.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q28/extended.txt (1)

1-78: LGTM! Test expectations correctly updated.

The updated execution plan correctly reflects the improved fallback reporting behavior from the main code changes.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q28/extended.txt (1)

1-78: LGTM! Test expectations correctly updated.

Consistent plan updates across all test variants, confirming the refactoring works correctly across different Spark versions.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4/q88/extended.txt (1)

1-216: LGTM! Test expectations correctly updated for q88.

The expanded plan (216 lines) shows comprehensive operator hierarchy with detailed Comet operators and accurate acceleration metrics. The changes are consistent with the improved fallback reporting introduced in this PR.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88.native_iceberg_compat/extended.txt (1)

1-216: Test expectations updated to reflect improved fallback reporting and plan visibility.

The expanded plan trace now shows detailed operator-level acceleration information, replacing previous compact "not supported" annotations. The summary metric (192/206 operators, 93% acceleration) provides clear validation of Comet's query execution coverage for this test case.

spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark4_0/q88/extended.txt (1)

1-216: Test expectations updated consistently with q88.native_iceberg_compat variant.

This file contains an identical plan representation to the native_iceberg_compat variant, which is expected if both test cases produce the same execution plan. The detailed operator tree and acceleration metrics (93%) align with the PR's focus on improved fallback reporting.

Please confirm that the identical plan content between q88 and q88.native_iceberg_compat is intentional. If these test cases should differ, we should verify the plan generation is correct.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review for PR #36

Summary

This PR refactors CometExecRule to improve fallback reporting and remove redundant code. The main changes involve eliminating the convertToCometIfAllChildrenAreNative helper method and improving the logic for when fallback messages are shown.

---

✅ Strengths

1. Good Code Simplification: Removing convertToCometIfAllChildrenAreNative (lines 459-470 deleted) reduces duplication since the child checking logic is now inline where it's used.

2. Improved Fallback Reporting: The new logic at lines 270-271 ensures fallback reasons are only added for operators whose children are all native AND don't already have explain info. This prevents redundant/misleading messages for operators that fall back because their children already fell back.

3. Better Separation of Concerns: Moving ExecutedCommandExec and V2CommandExec to the main exclusion list (line 257-258) is cleaner and more maintainable.

4. Consistent Test Updates: The test plan updates correctly reflect that intermediate BroadcastNestedLoopJoin nodes no longer get fallback messages since their children aren't all native.

---

🔍 Issues & Concerns

1. Potential Logic Bug in BroadcastExchangeExec Handling ⚠️

Location: CometExecRule.scala:208

case b: BroadcastExchangeExec if b.children.forall(_.isInstanceOf[CometNativeExec]) =>
  convertToComet(b, CometBroadcastExchangeExec).getOrElse(b)

Before: The old code called convertToCometIfAllChildrenAreNative which checked children inside the conversion method.
After: The check is now in the pattern match guard.

Issue: This is functionally equivalent, but there's a subtle difference in when the check happens. The old method would check children inside convertToComet, while now it's checked before. This should be fine, but worth validating that convertToComet doesn't make additional assumptions about when it's called.

2. Change in convertToComet Condition ⚠️

Location: CometExecRule.scala:464

if (op.children.nonEmpty && op.children.forall(_.isInstanceOf[CometNativeExec])) {

Before: Only checked if children were native
After: Also checks op.children.nonEmpty

Question: Is this check necessary? If op.children.forall(...) is evaluated on an empty collection, it returns true by default (vacuous truth). This means:

• Old behavior: Leaf nodes (empty children) would pass the forall check and try to add children
• New behavior: Leaf nodes skip the child-adding logic

Analysis: This appears to be a bug fix preventing unnecessary work for leaf nodes, but it should be verified that leaf operators don't break. The else branch (line 471-473) should handle leaf nodes correctly.

3. Improved Fallback Logic Needs Validation 📋

Location: CometExecRule.scala:270-271

if (op.children.forall(_.isInstanceOf[CometNativeExec]) && !hasExplainInfo(op)) {
  withInfo(op, s"${op.nodeName} is not supported")
}

This is a significant improvement, but we should verify:

• Does hasExplainInfo correctly identify all cases where fallback reasons were already set?
• Could there be edge cases where an operator has all native children but legitimately can't be converted, and this wouldn't be caught?

---

🎯 Performance Considerations

No concerns: The refactoring should have neutral or slightly positive performance impact:

• Removes one method call indirection
• The forall check now happens in fewer places
• No algorithmic changes

---

🔒 Security Concerns

No issues identified: This is internal query planning logic with no external input handling.

---

🧪 Test Coverage

Adequate but could be better:

• ✅ Test plan stability files updated correctly (12 files)
• ✅ The changes to expected output align with the code changes
• ⚠️ Missing: No unit tests specifically for the refactored methods
• ⚠️ Missing: No test coverage for the edge case of leaf operators with the new nonEmpty check

Recommendation: Consider adding a unit test that validates:

1. Leaf operators are handled correctly
2. The fallback message logic works as intended (operators with non-native children don't get redundant messages)

---

📝 Code Quality & Best Practices

1. Good: The refactoring improves code clarity
2. Good: Comments at lines 263-269 explain the three reasons why conversion might not happen
3. Good: The change consolidates the guard conditions making them easier to understand
4. Minor: Consider adding a code comment explaining why nonEmpty check was added at line 464

---

✅ Recommendations

1. Add a comment at line 464 explaining the nonEmpty check:

   // Only add children if the operator has children and they are all native.
   // Empty children (leaf nodes) are handled in the else branch.
   if (op.children.nonEmpty && op.children.forall(_.isInstanceOf[CometNativeExec])) {

2. Verify leaf node handling: Ensure that scan operators and other leaf nodes work correctly with the new nonEmpty check

3. Consider adding tests: Specifically for the fallback message improvements

---

📊 Overall Assessment

Verdict: ✅ Approve with minor suggestions

This is a solid refactoring that improves code quality and fixes fallback reporting. The logic changes appear correct, though the nonEmpty addition should be validated against leaf operators. The test updates are consistent with the code changes.

Risk Level: Low - This is primarily a refactoring with improved behavior, and existing tests should catch any regressions.

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.