chore: Improve reporting of fallback reasons for CollectLimit #1694
Conversation
| if (!isCometShuffleEnabled(conf)) { | ||
| fallbackReasons += "Comet shuffle is not enabled" | ||
| } |
There was a problem hiding this comment.
I'm actually not sure why we do not support CollectLimitExec if Comet shuffle is disabled 🤔
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1694 +/- ##
============================================
+ Coverage 56.12% 58.75% +2.62%
- Complexity 976 1135 +159
============================================
Files 119 129 +10
Lines 11743 12571 +828
Branches 2251 2350 +99
============================================
+ Hits 6591 7386 +795
- Misses 4012 4024 +12
- Partials 1140 1161 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (fallbackReasons.nonEmpty) { | ||
| withInfos(op, fallbackReasons.toSet) | ||
| } else { | ||
| if (!isCometNative(op.child)) { | ||
| // no reason to report reason if child is not native | ||
| op |
There was a problem hiding this comment.
hmmm is it possible that there are fallbackReasons as well as the child is not native at the same time? should we report that the child is not native then?
There was a problem hiding this comment.
'Child is not native' reporting was removed because it led to lots of repetitive messages. The child that is not native would have already reported the reason (I think).
Which issue does this PR close?
Part of #1669
Rationale for this change
I found it confusing to see
CollectLimitExec is not supported, because I know that it is supported (in some cases).What changes are included in this PR?
Improve reporting of fallback reasons.
How are these changes tested?