You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "szehon-ho (via GitHub)" <gi...@apache.org> on 2025/09/17 00:27:05 UTC
[PR] [SPARK-52991][SQL][FOLLOW-UP] Small optimization for MERGE INTO with SCHEMA EVOLUTION and add test [spark]
szehon-ho opened a new pull request, #52362:
URL: https://github.com/apache/spark/pull/52362
### What changes were proposed in this pull request?
Minor follow up for https://github.com/apache/spark/pull/51698
1. Small optimization for MergeIntoTable node (@aokolnychyi noticed post-commit that the new states can be calculated once using `lazy val`, as each time rules will copy the node so the state never changes).
2. Add test to validate that default values work in schema evolution case.
### Why are the changes needed?
Small analysis performance improvement and increase test coverage
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Run existing tests, and the patch adds another test
### Was this patch authored or co-authored using generative AI tooling?
No
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Small optimization for MERGE INTO WITH SCHEMA EVOLUTION and add test [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #52362:
URL: https://github.com/apache/spark/pull/52362#discussion_r2354327428
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -907,6 +902,12 @@ case class MergeIntoTable(
case _ => false
}
}
+
+ override def left: LogicalPlan = targetTable
+ override def right: LogicalPlan = sourceTable
+ override protected def withNewChildrenInternal(
+ newLeft: LogicalPlan, newRight: LogicalPlan): MergeIntoTable =
+ copy(targetTable = newLeft, sourceTable = newRight)
Review Comment:
Hi @dongjoon-hyun thanks for looking at it, yes its a minor cleanup, i noticed the other lazy vals are above the left, right, etc. I can revert it tomorrow if its an issue
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Revise `MergeIntoTable` to use `lazy val` and add a new test [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #52362:
URL: https://github.com/apache/spark/pull/52362#issuecomment-3301360904
Thank you, @dongjoon-hyun !
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Small optimization for MERGE INTO WITH SCHEMA EVOLUTION and add test [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #52362:
URL: https://github.com/apache/spark/pull/52362#discussion_r2354327428
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -907,6 +902,12 @@ case class MergeIntoTable(
case _ => false
}
}
+
+ override def left: LogicalPlan = targetTable
+ override def right: LogicalPlan = sourceTable
+ override protected def withNewChildrenInternal(
+ newLeft: LogicalPlan, newRight: LogicalPlan): MergeIntoTable =
+ copy(targetTable = newLeft, sourceTable = newRight)
Review Comment:
Hi @dongjoon-hyun thanks for looking at it, yes its a minor cleanup, i noticed the other lazy vals are above the left, right, etc and move these ones to join them. I can revert it tomorrow if its an issue
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Small optimization for MERGE INTO WITH SCHEMA EVOLUTION and add test [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52362:
URL: https://github.com/apache/spark/pull/52362#discussion_r2354222501
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -907,6 +902,12 @@ case class MergeIntoTable(
case _ => false
}
}
+
+ override def left: LogicalPlan = targetTable
+ override def right: LogicalPlan = sourceTable
+ override protected def withNewChildrenInternal(
+ newLeft: LogicalPlan, newRight: LogicalPlan): MergeIntoTable =
+ copy(targetTable = newLeft, sourceTable = newRight)
Review Comment:
Hi, @szehon-ho . May I ask why you propose to move the above 5 lines as an optimization? This doesn't look like an optimization. If this is a simple relocation, I'd ask you revert this change from this PR because this PR's title is `Small optimization for ...`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Revise `MergeIntoTable` to use `lazy val` and add a new test [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #52362: [SPARK-52991][SQL][FOLLOW-UP] Revise `MergeIntoTable` to use `lazy val` and add a new test
URL: https://github.com/apache/spark/pull/52362
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Revise `MergeIntoTable` to use `lazy val` and add a new test [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52362:
URL: https://github.com/apache/spark/pull/52362#issuecomment-3301357356
Merged to master for Apache Spark 4.1.0-preview2.
Thank you, @szehon-ho .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52991][SQL][FOLLOW-UP] Small optimization for MERGE INTO WITH SCHEMA EVOLUTION and add test [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52362:
URL: https://github.com/apache/spark/pull/52362#discussion_r2354333221
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala:
##########
@@ -907,6 +902,12 @@ case class MergeIntoTable(
case _ => false
}
}
+
+ override def left: LogicalPlan = targetTable
+ override def right: LogicalPlan = sourceTable
+ override protected def withNewChildrenInternal(
+ newLeft: LogicalPlan, newRight: LogicalPlan): MergeIntoTable =
+ copy(targetTable = newLeft, sourceTable = newRight)
Review Comment:
Got it. No problem. Let me revise the PR title and description a little and merge this, @szehon-ho. Thank you.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org