CodiumAI-Agent commented on PR #9270: URL: https://github.com/apache/incubator-gluten/pull/9270#issuecomment-2795738925
## PR Code Suggestions ✨ <!-- 6c7c5fc --> <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 property restoration consistency</summary> ___ **Ensure that the restoration of the topmost conversion property is executed even when <br>early returns occur.** [backends-clickhouse/src-delta-32/main/scala/org/apache/spark/sql/delta/commands/merge/MergeIntoMaterializeSource.scala [292-343]](https://github.com/apache/incubator-gluten/pull/9270/files#diff-28fea08979470aef04fa2e5adc3973313a3c3f280175d8265a6ebc5a1e5c9a6aR292-R343) ```diff // --- modified start val originalRemoveTopmostC2R = CHRemoveTopmostColumnarToRow.isRemoveTopmostC2R(spark) -spark.sparkContext.setLocalProperty( - CHRemoveTopmostColumnarToRow.REMOVE_TOPMOST_COLUMNAR_TO_ROW, - "true") +try { + spark.sparkContext.setLocalProperty( + CHRemoveTopmostColumnarToRow.REMOVE_TOPMOST_COLUMNAR_TO_ROW, + "true") + if (!materialize) { + mergeSource = Some( + MergeSource( + df = Dataset.ofRows(spark, source), + isMaterialized = false, + materializeReason = materializeReason + ) + ) + return + } + // Continue with materialization logic... + ... +} finally { + CHRemoveTopmostColumnarToRow.setRemoveTopmostC2R(originalRemoveTopmostC2R, spark) +} // --- modified end -if (!materialize) { - // Does not materialize, simply return the dataframe from source plan - mergeSource = Some( - MergeSource( - df = Dataset.ofRows(spark, source), - isMaterialized = false, - materializeReason = materializeReason - ) - ) - return -} -... -// --- modified start -CHRemoveTopmostColumnarToRow.setRemoveTopmostC2R(originalRemoveTopmostC2R, spark) -// --- modified end - ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion addresses a potential bug where an early return might skip the restoration of the topmost conversion property. Wrapping the configuration change in a try–finally block ensures that the property is reset regardless of control flow, making the solution both relevant and effective. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Improve streaming detection</summary> ___ **Review the string matching logic for detecting streaming calls to ensure it <br>accurately identifies all relevant call frames.** [gluten-core/src/main/scala/org/apache/gluten/extension/caller/CallerInfo.scala [69-71]](https://github.com/apache/incubator-gluten/pull/9270/files#diff-a2b89cccbe3635d4874868fb9364d87f837a2ee236a7b0adadc9d3ecb30cbf7dR69-R71) ```diff private def inStreamingCall(stack: Seq[StackTraceElement]): Boolean = { - stack.exists(_.getClassName.equals(StreamExecution.getClass.getName.split('$').head)) + // Consider using a more robust matching method, e.g., checking for substring or regex that matches expected streaming classes. + stack.exists(elem => elem.getClassName.contains("StreamExecution")) } ``` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion enhances the existing string matching by using a substring check which is more flexible; however, as it is an improvement in detection logic rather than a fix for a critical bug, its impact is moderate and may risk false positives if not carefully reviewed. </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]
