Copilot commented on code in PR #12626:
URL: https://github.com/apache/gluten/pull/12626#discussion_r3833831191
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala:
##########
@@ -238,4 +264,94 @@ abstract class FileSourceScanExecTransformerBase(
object FileSourceScanExecTransformerBase {
private def isDynamicPruningFilter(e: Expression): Boolean =
e.find(_.isInstanceOf[PlanExpression[_]]).isDefined
+
+ // Mark each pushed filter with a `*` in the rendered `PushedFilters` list,
mirroring Spark's
+ // RowDataSourceScanExec convention for a filter the source evaluates itself
(so no separate
+ // post-scan Filter is needed). Gluten pushes every conjunct into the native
scan via
+ // PushDownFilterToScan and applies them as exact row-level filters, so the
paired
+ // FilterExecTransformer is a no-op (FilterExecTransformerBase.isNoop) --
the same end state a `*`
+ // advertises. `metadata` is a lazy val and cannot be super-overridden, so
the mark is applied to
+ // the rendered plan string.
+
+ /**
+ * Mark every entry of the `PushedFilters: [...]` list in a rendered
plan-node string with a `*`.
+ * Returns the text unchanged when no (untruncated) `PushedFilters` list is
present.
+ */
+ private[execution] def markPushedFilters(text: String): String =
+ rewritePushedFiltersList(text, starPushedFilters)
+
+ /**
+ * Locate the `PushedFilters: [...]` list in a rendered plan-node string and
replace it with
+ * `rewrite(list)` (where `list` is the whole `"[...]"` including brackets).
Returns the text
+ * unchanged when no (untruncated) `PushedFilters` list is present.
+ */
+ private[execution] def rewritePushedFiltersList(
+ text: String,
+ rewrite: String => String): String = {
+ val marker = "PushedFilters: ["
+ val markerAt = text.indexOf(marker)
+ if (markerAt < 0) {
+ return text
+ }
+ val listStart = markerAt + marker.length - 1 // index of the opening '['
+ var depth = 0
+ var i = listStart
+ var listEnd = -1
+ while (i < text.length && listEnd < 0) {
+ text.charAt(i) match {
+ case '[' | '(' => depth += 1
+ case ']' | ')' =>
+ depth -= 1
+ if (depth == 0) {
+ listEnd = i
+ }
+ case _ =>
+ }
Review Comment:
`rewritePushedFiltersList` uses a single `depth` counter for both `[]` and
`()`. If a pushed-filter rendering contains a literal `)` (e.g., in a quoted
column name or string literal) without a matching `(`, the counter can reach
zero before the closing `]`, causing the rewrite to treat that `)` as the end
of the PushedFilters list and potentially corrupt the rendered plan.
Consider tracking only `[`/`]` for locating the list end (parentheses don’t
affect where the top-level `]` is) to avoid premature termination.
--
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]
