CodiumAI-Agent commented on PR #8706: URL: https://github.com/apache/incubator-gluten/pull/8706#issuecomment-2652544092
## PR Reviewer Guide ๐ Here are some key observations to aid the review process: <table> <tr><td> **๐ซ Ticket compliance analysis ๐ถ** **[8705](https://github.com/apache/incubator-gluten/issues/8705) - Partially compliant** Compliant requirements: - Enable `MemorySpillScheduler` to help avoid OOM. Non-compliant requirements: [] Requires further human verification: [] </td></tr> <tr><td>โฑ๏ธ <strong>Estimated effort to review</strong>: 4 ๐ต๐ต๐ต๐ตโช</td></tr> <tr><td>๐งช <strong>No relevant tests</strong></td></tr> <tr><td>๐ <strong>No security concerns identified</strong></td></tr> <tr><td>โก <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/8706/files#diff-ee3ceaff98034c1144e0dc6255652ca1c39ea970837418e240409ccb05622d00R102-R102'><strong>Possible Issue</strong></a> The setting `enable_adaptive_memory_spill_scheduler` is hardcoded to `true`. Consider whether this should be configurable or if there are scenarios where this might cause unintended behavior.</summary> ```c++ query_context->query_context->setSetting("enable_adaptive_memory_spill_scheduler", true); ``` </details> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/8706/files#diff-e735db3b50a5f38423fd316e70c4e5a84d3908b691c93532def47a4e61baeee8R594-R618'><strong>Performance Concern</strong></a> The `getMemoryStats` function performs multiple calculations and iterations that could impact performance in high-load scenarios. Consider optimizing or caching results if this function is called frequently.</summary> ```c++ DB::ProcessorMemoryStats GraceAggregatingTransform::getMemoryStats() { DB::ProcessorMemoryStats stats; if (!current_data_variants) return stats; stats.need_reserved_memory_bytes = current_data_variants->aggregates_pool->allocatedBytes(); for (size_t i = current_bucket_index + 1; i < getBucketsNum(); ++i) { auto & file_stream = buckets[i]; stats.spillable_memory_bytes += file_stream.pending_bytes; } if (per_key_memory_usage > 0) { auto current_result_rows = current_data_variants->size(); stats.need_reserved_memory_bytes += current_result_rows * per_key_memory_usage; stats.spillable_memory_bytes += current_result_rows * per_key_memory_usage; } else { // This is a rough estimation, we don't know the exact memory usage for each key. stats.spillable_memory_bytes += current_data_variants->aggregates_pool->allocatedBytes(); } return stats; ``` </details> <details><summary><a href='https://github.com/apache/incubator-gluten/pull/8706/files#diff-e735db3b50a5f38423fd316e70c4e5a84d3908b691c93532def47a4e61baeee8R621-R629'><strong>Possible Logic Issue</strong></a> The `spillOnSize` method uses a threshold of `bytes * 0.8` to determine whether to trigger a spill. Ensure this threshold is appropriate and won't lead to premature or delayed spills in various scenarios.</summary> ```c++ bool GraceAggregatingTransform::spillOnSize(size_t bytes) { auto stats = getMemoryStats(); if (stats.spillable_memory_bytes < bytes * 0.8) return false; force_spill = true; force_spill_on_bytes = bytes; return true; } ``` </details> </td></tr> </table> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
