cloud-fan commented on code in PR #56930:
URL: https://github.com/apache/spark/pull/56930#discussion_r3510696020
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveBinBySuite.scala:
##########
@@ -202,6 +202,35 @@ class ResolveBinBySuite extends AnalysisTest {
assert(bi.distributeColumns.map(_.exprId) == Seq(value.exprId))
}
+ test("resolved BinBy emits the DISTRIBUTE column as a produced attribute
replacing the input") {
Review Comment:
The PR description lists "multiple `DISTRIBUTE` columns are each replaced in
place with distinct fresh ids" as a tested case, but this suite has no such
test: this one resolves a single column and asserts
`scaledDistributeColumns.map(_.exprId) == Seq(outValue.exprId)`, and the only
multi-element `distribute` list (`Seq(value, value)` at line 322) is the
duplicate-rejection negative test.
Multi-column is the case `distributeReplacements` (an `AttributeMap` over a
zip) and `child.output.map(getOrElse)` exist to serve — worth a test with two
distinct DISTRIBUTE columns at different schema positions, asserting each is
replaced in place with a distinct fresh id. Non-blocking, but it closes the gap
and the stale description claim.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala:
##########
@@ -1822,25 +1828,27 @@ case class BinBy(
s"${rangeStart.dataType}, timeZoneId=$timeZoneId")
}
- override def output: Seq[Attribute] = child.output ++ appendedAttributes
+ assert(distributeColumns.length == scaledDistributeColumns.length,
Review Comment:
Minor: the `timeZoneId` invariant right above (line 1826) throws
`SparkException.internalError`, while this equal-length invariant uses
`assert`. Both are reasonable, but two internal-invariant mechanisms side by
side in the same constructor reads slightly inconsistently — consider matching
the neighbor for uniformity.
--
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]
