Re: [PR] [GLUTEN-9085][CH] Add UT for mergetree write stats [incubator-gluten]

[via GitHub]

CodiumAI-Agent commented on PR #9089:
URL: 
https://github.com/apache/incubator-gluten/pull/9089#issuecomment-2746794976

   ## PR Reviewer Guide 🔍
   
   Here are some key observations to aid the review process:
   
   <table>
   <tr><td>
   
   **🎫 Ticket compliance analysis 🔶**
   
   
   
   **[9085](https://github.com/apache/incubator-gluten/issues/9085) - Partially 
compliant**
   
   Compliant requirements:
   
   - Added unit test for mergetree write stats.
   - Updated DeltaStats to cap the number of stats columns based on 
MAX_STATS_COLS.
   
   Non-compliant requirements:
   
   - None
   
   Requires further human verification:
   
   - Verify that the new loop conditions in DeltaStats functions do not cause 
unintended infinite loops.
   
   
   
   </td></tr>
   <tr><td>⏱️&nbsp;<strong>Estimated effort to review</strong>: 3 
🔵🔵🔵⚪⚪</td></tr>
   <tr><td>🧪&nbsp;<strong>PR contains tests</strong></td></tr>
   <tr><td>🔒&nbsp;<strong>No security concerns identified</strong></td></tr>
   <tr><td>⚡&nbsp;<strong>Recommended focus areas for review</strong><br><br>
   
   <details><summary><a 
href='https://github.com/apache/incubator-gluten/pull/9089/files#diff-6023bcc1d7f3a706c2eb3bd78dafb33462cddc0030d05a1d3eb61941bd41d73fR96-R119'><strong>Loop
 Condition Concern</strong></a>
   
   The newly added for loops in the DeltaStats::statsHeader and update methods 
use an '||' condition which may lead to unexpected behaviors or potential 
infinite loops if the column count conditions are not met as expected.
   </summary>
   
   ```c
   size_t num_stats_cols = numStatsCols(output.columns() - partition.size());
   auto appendBase = [&](const std::string & prefix)
   {
       for (size_t i = 0, n = 0; i < output.columns() || n < num_stats_cols; 
i++)
       {
           const auto & column = output.getByPosition(i);
           if (!partition_index.contains(column.name))
           {
               statsHeaderBase.emplace_back(wrapNullableType(column.type), 
prefix + column.name);
               ++n;
           }
       }
   };
   appendBase("min_");
   appendBase("max_");
   for (size_t i = 0, n = 0; i < output.columns() || n < num_stats_cols; i++)
   {
       const auto & column = output.getByPosition(i);
       if (!partition_index.contains(column.name))
       {
           statsHeaderBase.emplace_back(BIGINT(), "null_count_" + column.name);
           ++n;
       }
   }
   ```
   
   </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: commits-unsubscr...@gluten.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@gluten.apache.org
For additional commands, e-mail: commits-h...@gluten.apache.org