You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vladimirg-db (via GitHub)" <gi...@apache.org> on 2025/01/06 18:50:34 UTC
[PR] [SPARK-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
vladimirg-db opened a new pull request, #49377:
URL: https://github.com/apache/spark/pull/49377
### What changes were proposed in this pull request?
Normalize CTERelationDef and CTERelationRef IDs.
### Why are the changes needed?
This is necessary so that the output of single-pass Analyzer containing CTEs can be compared to the fixed-point one.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing tests.
### 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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on PR #49377:
URL: https://github.com/apache/spark/pull/49377#issuecomment-2575112069
@cloud-fan great idea. Done.
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #49377:
URL: https://github.com/apache/spark/pull/49377#issuecomment-2576755763
thanks, merging to master!
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #49377:
URL: https://github.com/apache/spark/pull/49377#discussion_r1906981407
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/NormalizePlan.scala:
##########
@@ -143,3 +154,25 @@ object NormalizePlan extends PredicateHelper {
case _ => condition // Don't reorder.
}
}
+
+class CteIdNormalizer {
+ private var cteIdCounter: Long = 0
+ private val oldToNewIdMapping = new HashMap[Long, Long]
+
+ def normalizeDef(cteRelationDef: CTERelationDef): CTERelationDef = {
+ try {
+ oldToNewIdMapping.put(cteRelationDef.id, cteIdCounter)
+ cteRelationDef.copy(id = cteIdCounter)
+ } finally {
+ cteIdCounter += 1
+ }
+ }
+
+ def normalizeRef(cteRelationRef: CTERelationRef): CTERelationRef = {
+ if (oldToNewIdMapping.containsKey(cteRelationRef.cteId)) {
+ cteRelationRef.copy(cteId = oldToNewIdMapping.get(cteRelationRef.cteId))
+ } else {
+ cteRelationRef
Review Comment:
Test output is usually very noisy, and it seems to me that nobody would notice this warning. @cloud-fan do you think a log here would add some value?
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #49377:
URL: https://github.com/apache/spark/pull/49377#discussion_r1907247603
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/NormalizePlan.scala:
##########
@@ -143,3 +154,25 @@ object NormalizePlan extends PredicateHelper {
case _ => condition // Don't reorder.
}
}
+
+class CteIdNormalizer {
+ private var cteIdCounter: Long = 0
+ private val oldToNewIdMapping = new HashMap[Long, Long]
+
+ def normalizeDef(cteRelationDef: CTERelationDef): CTERelationDef = {
+ try {
+ oldToNewIdMapping.put(cteRelationDef.id, cteIdCounter)
+ cteRelationDef.copy(id = cteIdCounter)
+ } finally {
+ cteIdCounter += 1
+ }
+ }
+
+ def normalizeRef(cteRelationRef: CTERelationRef): CTERelationRef = {
+ if (oldToNewIdMapping.containsKey(cteRelationRef.cteId)) {
+ cteRelationRef.copy(cteId = oldToNewIdMapping.get(cteRelationRef.cteId))
+ } else {
+ cteRelationRef
Review Comment:
Given the above, we should throw exception in the `else` branch.
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #49377:
URL: https://github.com/apache/spark/pull/49377#issuecomment-2574292610
shall we reassign the CTE id from 0 in `NormalizePlan`? I feel it's risky to normalize CTE ids to 0, as we lose the information of the CTE reference connection.
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #49377:
URL: https://github.com/apache/spark/pull/49377#discussion_r1906959506
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/NormalizePlan.scala:
##########
@@ -143,3 +154,25 @@ object NormalizePlan extends PredicateHelper {
case _ => condition // Don't reorder.
}
}
+
+class CteIdNormalizer {
+ private var cteIdCounter: Long = 0
+ private val oldToNewIdMapping = new HashMap[Long, Long]
+
+ def normalizeDef(cteRelationDef: CTERelationDef): CTERelationDef = {
+ try {
+ oldToNewIdMapping.put(cteRelationDef.id, cteIdCounter)
+ cteRelationDef.copy(id = cteIdCounter)
+ } finally {
+ cteIdCounter += 1
+ }
+ }
+
+ def normalizeRef(cteRelationRef: CTERelationRef): CTERelationRef = {
+ if (oldToNewIdMapping.containsKey(cteRelationRef.cteId)) {
+ cteRelationRef.copy(cteId = oldToNewIdMapping.get(cteRelationRef.cteId))
+ } else {
+ cteRelationRef
Review Comment:
Should a `warn` log be added here ?
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #49377:
URL: https://github.com/apache/spark/pull/49377#discussion_r1906966079
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/NormalizePlan.scala:
##########
@@ -143,3 +154,25 @@ object NormalizePlan extends PredicateHelper {
case _ => condition // Don't reorder.
}
}
+
+class CteIdNormalizer {
+ private var cteIdCounter: Long = 0
+ private val oldToNewIdMapping = new HashMap[Long, Long]
+
+ def normalizeDef(cteRelationDef: CTERelationDef): CTERelationDef = {
+ try {
+ oldToNewIdMapping.put(cteRelationDef.id, cteIdCounter)
+ cteRelationDef.copy(id = cteIdCounter)
+ } finally {
+ cteIdCounter += 1
+ }
+ }
+
+ def normalizeRef(cteRelationRef: CTERelationRef): CTERelationRef = {
+ if (oldToNewIdMapping.containsKey(cteRelationRef.cteId)) {
+ cteRelationRef.copy(cteId = oldToNewIdMapping.get(cteRelationRef.cteId))
+ } else {
+ cteRelationRef
Review Comment:
You are describing current development.
Even for failed tests, a `warn` would give us some information - let alone there is a chance of coverage issue several months down the road.
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #49377:
URL: https://github.com/apache/spark/pull/49377#discussion_r1907247326
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/NormalizePlan.scala:
##########
@@ -143,3 +154,25 @@ object NormalizePlan extends PredicateHelper {
case _ => condition // Don't reorder.
}
}
+
+class CteIdNormalizer {
+ private var cteIdCounter: Long = 0
+ private val oldToNewIdMapping = new HashMap[Long, Long]
+
+ def normalizeDef(cteRelationDef: CTERelationDef): CTERelationDef = {
+ try {
+ oldToNewIdMapping.put(cteRelationDef.id, cteIdCounter)
+ cteRelationDef.copy(id = cteIdCounter)
+ } finally {
+ cteIdCounter += 1
+ }
+ }
+
+ def normalizeRef(cteRelationRef: CTERelationRef): CTERelationRef = {
+ if (oldToNewIdMapping.containsKey(cteRelationRef.cteId)) {
+ cteRelationRef.copy(cteId = oldToNewIdMapping.get(cteRelationRef.cteId))
+ } else {
+ cteRelationRef
Review Comment:
Seems it's test only, maybe we should just fail here?
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "vladimirg-db (via GitHub)" <gi...@apache.org>.
vladimirg-db commented on code in PR #49377:
URL: https://github.com/apache/spark/pull/49377#discussion_r1906962536
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/NormalizePlan.scala:
##########
@@ -143,3 +154,25 @@ object NormalizePlan extends PredicateHelper {
case _ => condition // Don't reorder.
}
}
+
+class CteIdNormalizer {
+ private var cteIdCounter: Long = 0
+ private val oldToNewIdMapping = new HashMap[Long, Long]
+
+ def normalizeDef(cteRelationDef: CTERelationDef): CTERelationDef = {
+ try {
+ oldToNewIdMapping.put(cteRelationDef.id, cteIdCounter)
+ cteRelationDef.copy(id = cteIdCounter)
+ } finally {
+ cteIdCounter += 1
+ }
+ }
+
+ def normalizeRef(cteRelationRef: CTERelationRef): CTERelationRef = {
+ if (oldToNewIdMapping.containsKey(cteRelationRef.cteId)) {
+ cteRelationRef.copy(cteId = oldToNewIdMapping.get(cteRelationRef.cteId))
+ } else {
+ cteRelationRef
Review Comment:
No. `NormalizePlan` is used in Optimzer tests and to compare single-pass/fixed-point Analyzer plans. If we preserve the old ID here, the plan comparison would fail anyway, so you'll see it as a failed test.
--
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-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #49377: [SPARK-50743][SQL] Normalize CTERelationDef and CTERelationRef IDs
URL: https://github.com/apache/spark/pull/49377
--
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