szehon-ho commented on code in PR #58666:
URL: https://github.com/apache/spark/pull/58666#discussion_r4031537098
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala:
##########
@@ -602,11 +604,23 @@ object DataSourceV2Relation {
val catalystColStat = ColumnStat(distinct, min, max, nullCount,
avgLen, maxLen, histogram)
- output.foreach(attribute => {
- if (attribute.name.equals(key.describe())) {
- colStats = colStats :+ (attribute -> catalystColStat)
+ // Catalyst statistics are keyed by top-level Attribute, so only
single-part references
Review Comment:
Could we condense both comments to:
```scala
// Catalyst column statistics only support top-level attributes. Prefer a
unique exact name
// when the configured resolver matches multiple output attributes.
```
This keeps the invariant and ambiguity rationale without narrating the
branch.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2RelationSuite.scala:
##########
@@ -132,24 +132,57 @@ class DataSourceV2RelationSuite extends SparkFunSuite
with SQLHelper {
output)
}
+ private def optLong(value: Option[Long]): OptionalLong =
+ value.map(OptionalLong.of).getOrElse(OptionalLong.empty())
+
+ /** Builds a [[ColumnStatistics]] overriding only the fields these tests
exercise. */
+ private def columnStat(
+ distinct: Option[Long] = None,
+ avg: Option[Long] = None): ColumnStatistics = new ColumnStatistics {
+ override def distinctCount(): OptionalLong = optLong(distinct)
+ override def avgLen(): OptionalLong = optLong(avg)
+ }
+
+ /** Builds a column-stats map keyed by [[NamedReference]] from `name ->
stats` pairs. */
+ private def columnStatsMap(
+ entries: (String, ColumnStatistics)*): java.util.Map[NamedReference,
ColumnStatistics] = {
Review Comment:
Since this file also uses Scala `Map`, could we alias the Java type in the
imports (`Map => JMap`) and import `HashMap` directly, then use
`JMap`/`HashMap` here and below?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2RelationSuite.scala:
##########
@@ -132,24 +132,57 @@ class DataSourceV2RelationSuite extends SparkFunSuite
with SQLHelper {
output)
}
+ private def optLong(value: Option[Long]): OptionalLong =
+ value.map(OptionalLong.of).getOrElse(OptionalLong.empty())
+
+ /** Builds a [[ColumnStatistics]] overriding only the fields these tests
exercise. */
Review Comment:
These private helper Scaladocs mostly restate the signatures. Could we
remove them and use more specific names such as `v2ColumnStat`,
`v2ColumnStatsMap`, and `v2Statistics`? For `newStatsScan`, the only
non-obvious point worth retaining is: `/** Evaluates stats for each
estimateStatistics() call. */`
--
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]
