CodiumAI-Agent commented on PR #9142: URL: https://github.com/apache/incubator-gluten/pull/9142#issuecomment-2756953454
## PR Code Suggestions ✨ <!-- b7976a1 --> <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>Ensure table cleanup</summary> ___ **Wrap the test operations in a try-finally block so that the DROP statement is always <br>executed even when an exception occurs.** [backends-clickhouse/src/test/scala/org/apache/gluten/execution/hive/GlutenClickHouseHiveTableSuite.scala [1679-1683]](https://github.com/apache/incubator-gluten/pull/9142/files#diff-7c3a2853de5904b6c488003be4b92b2ec77b067a550e5294fc42b9cf05d8b54aR1679-R1683) ```diff spark.sql(createTableSql) -spark.sql(insertDataSql1) -spark.sql(insertDataSql2) -compareResultsAgainstVanillaSpark(selectSql, compareResult = true, _ => {}) -spark.sql(dropSql) +try { + spark.sql(insertDataSql1) + spark.sql(insertDataSql2) + compareResultsAgainstVanillaSpark(selectSql, compareResult = true, _ => {}) +} finally { + spark.sql(dropSql) +} ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: Wrapping the test operations in a try-finally block ensures that the drop statement is always executed, which is important for test cleanup even if exceptions occur. This is a relevant and practical enhancement to maintain test reliability. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Align output attribute handling</summary> ___ **For consistency with similar cases, consider updating the output attribute in the <br>copied plan, for example by passing <code>p.output</code> as the output.** [gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/PushDownInputFileExpression.scala [120-127]](https://github.com/apache/incubator-gluten/pull/9142/files#diff-d350dba2dc24e5c89b32766f8ab3c2f7eb4e3131b7625080a545d98f44f69ec3R120-R127) ```diff case p @ ProjectExec(projectList, child: HiveTableScanExecTransformer) if projectList.exists(containsInputFileRelatedExpr) => child.copy( + output = p.output, requestedAttributes = p.output, relation = child.relation, partitionPruningPred = child.partitionPruningPred, prunedOutput = child.prunedOutput )(child.session) ``` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion aims to maintain consistency with similar cases by updating the output attribute; however, it may not be necessary if the existing implementation is sufficient, and it risks introducing issues if the child.copy method does not support an 'output' parameter. </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]
