dongjoon-hyun commented on code in PR #57443:
URL: https://github.com/apache/spark/pull/57443#discussion_r3658966153
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala:
##########
@@ -242,6 +242,18 @@ trait LeafNode extends LogicalPlan with
LeafLike[LogicalPlan] {
throw new SparkUnsupportedOperationException("_LEGACY_ERROR_TEMP_3114")
}
+/** A materialized leaf whose output can safely be scanned again to build a
runtime filter. */
+private[sql] trait LeafNodeWithAccurateStats extends LeafNode {
+ /** Whether the current materialized output has complete, accurate
statistics. */
+ def statsAvailable: Boolean
+
+ /** Whether scanning the materialized output again returns the same rows. */
+ def isOutputRepeatable: Boolean
+
+ /** Whether the original plan contains a predicate that is likely to be
selective. */
+ def hasSelectivePredicate: Boolean
+}
Review Comment:
Two naming/documentation concerns on this trait:
1. `statsAvailable`'s doc says "the current materialized output has
complete, accurate
statistics", but the `InMemoryRelation` implementation additionally
requires
`storageLevel.useDisk`. A fully materialized `MEMORY_ONLY` cache does
have complete, accurate
statistics (the new tests even assert `isOutputRepeatable == true` for
it) — the disk
requirement is about durability of the materialized output for a later
re-scan, not about the
accuracy of the stats. Could we either document that (e.g. "statistics
that will remain valid
when the output is scanned again, which excludes storage levels whose
blocks can be silently
evicted") or split the durability condition into its own method so each
predicate means exactly
what its name says?
2. The trait name advertises "accurate stats", but its contract also carries
repeatability
(`isOutputRepeatable`) and a runtime-filter-specific profitability hint
(`hasSelectivePredicate`) into `catalyst`'s `LogicalPlan.scala`. I
understand the module
boundary forces some interface here (the catalyst rule cannot see
`InMemoryRelation` in
`sql/core`), but a name like `MaterializedLeafNode` plus a class-level
doc stating that this is
the contract consumed by `InjectRuntimeFilter` would make the intent
clearer to the next reader
than the current stats-centric name.
--
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]
