uros-b commented on code in PR #58272:
URL: https://github.com/apache/spark/pull/58272#discussion_r3857161145
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala:
##########
@@ -85,23 +84,12 @@ abstract class DataSourceV2RelationBase(
s"RelationV2$outputString $nameWithTimeTravelSpec"
}
- override def computeStats(): Statistics = {
- if (Utils.isTesting) {
- // when testing, throw an exception if this computeStats method is
called because stats should
- // not be accessed before pushing the projection and filters to create a
scan. otherwise, the
- // stats are not accurate because they are based on a full table scan of
all columns.
- throw SparkException.internalError(
- s"BUG: computeStats called before pushdown on DSv2 relation: $name")
- } else {
- // when not testing, return stats because bad stats are better than
failing a query
- table.asReadable.newScanBuilder(options).build() match {
- case r: SupportsReportStatistics =>
- val statistics = r.estimateStatistics()
- DataSourceV2Relation.transformV2Stats(statistics,
conf.defaultSizeInBytes, output)
- case _ =>
- Statistics(sizeInBytes = conf.defaultSizeInBytes)
- }
- }
+ override def computeStats(): Statistics = table match {
+ case t: SupportsReportCatalogStatistics =>
+ DataSourceV2Relation.transformV2Stats(
+ t.estimateCatalogStatistics(), conf.defaultSizeInBytes, output)
+ case _ =>
+ Statistics(sizeInBytes = conf.defaultSizeInBytes)
}
Review Comment:
@hemanthboyina Could you please check:
The PR removes the production fallback that called
table.asReadable.newScanBuilder(options).build() and used
SupportsReportStatistics stats if the scan supported them. Post-PR, all
connectors that have not yet implemented SupportsReportCatalogStatistics,
including those that already implement SupportsReportStatistics on their scans
(e.g., Iceberg), silently fall back to conf.defaultSizeInBytes for the
PushDownLeftSemiAntiJoin estimation pass. In production, those connectors
previously supplied whole-table scan-level stats at this point; after the PR
they supply the fixed default, which for a large table could tip the planner
toward a broadcast join where a sort-merge would be safer. The design intent is
sound (the old scan-builder path was wasteful and pre-pushdown anyway), but the
PR should explicitly acknowledge this transition-period regression for existing
connectors, or if the migration risk is judged non-trivial, then consider
keeping the scan-builder path as a secondary fal
lback when the table implements SupportsReportStatistics but not the new
interface.
--
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]
