CodiumAI-Agent commented on PR #8929: URL: https://github.com/apache/incubator-gluten/pull/8929#issuecomment-2705850313
## PR Code Suggestions ✨ <!-- c3ad89d --> <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Add null pointer check</summary> ___ **Add a null pointer check for <code>arg0_col</code> before accessing its properties to avoid <br>potential segmentation faults.** [cpp-ch/local-engine/Parser/aggregate_function_parser/LeadLagParser.cpp [79]](https://github.com/apache/incubator-gluten/pull/8929/files#diff-9d20159ad1ceff2c6817603d3e10fe5dd5db0b20bd6cd45849a41d9d9deb2e68R79-R79) ```diff const auto * arg0_col = parseExpression(actions_dag, arg0); +if (!arg0_col) { + throw std::runtime_error("Failed to parse expression for arg0."); +} ``` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: This suggestion enhances robustness by ensuring that a null pointer is caught before being dereferenced; while not essential if parseExpression is guaranteed non-null, the added safety check can prevent potential runtime errors. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Remove duplicate tests</summary> ___ **Consolidate duplicate test cases or clearly differentiate their purposes to avoid <br>redundancy.** [backends-clickhouse/src/test/scala/org/apache/gluten/execution/compatibility/GlutenClickhouseFunctionSuite.scala [445-459]](https://github.com/apache/incubator-gluten/pull/8929/files#diff-5b7606eabf1b035656c88c98c30f15b55d67bc9fecf1463f0a6dd3d7c5498e1cR445-R459) ```diff -test("GLUTEN-8921: Type mismatch at checkDecimalOverflowSparkOrNull") { +test("GLUTEN-8921: Type mismatch - scenario 1") { compareResultsAgainstVanillaSpark( """ |select l_shipdate, avg(l_quantity), count(0) over() COU, |SUM(-1.1) over() SU, AVG(-2) over() AV, |max(-1.1) over() MA, min(-3) over() MI |from lineitem |where l_shipdate <= date'1998-09-02' |group by l_shipdate |order by l_shipdate """.stripMargin, true, { _ => } ) } +// Consider parameterizing similar tests into one. ``` <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion proposes renaming and consolidating similar test cases to reduce redundancy, which is a useful stylistic improvement but not critical to functionality. </details></details></td><td align=center>Low </td></tr></tr></tbody></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]
