dtenedor commented on code in PR #56468:
URL: https://github.com/apache/spark/pull/56468#discussion_r3553751836
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/FunctionResolverUtils.scala:
##########
@@ -119,17 +114,8 @@ trait FunctionResolverUtils {
.isTemporaryFunction(FunctionIdentifier(unresolvedFunction.nameParts.head))
}
- private def isCount(
- unresolvedFunction: UnresolvedFunction,
- normalizeFunctionName: Boolean = true
- ): Boolean = {
- val isCountName = if (normalizeFunctionName) {
- unresolvedFunction.nameParts.head.toLowerCase(Locale.ROOT) == "count"
- } else {
- unresolvedFunction.nameParts.head == "count"
- }
-
- unresolvedFunction.nameParts.length == 1 && isCountName
+ private def isCount(unresolvedFunction: UnresolvedFunction): Boolean = {
+
FunctionResolution.isUnqualifiedOrBuiltinFunctionName(unresolvedFunction.nameParts,
"count")
Review Comment:
For this switch from `assertSingleTableStarNotInCountFunction` to
case-insensitive `isUnqualifiedOrBuiltinFunctionName`: fixed-point still uses
case-sensitive `nameParts == Seq("count")` (`Analyzer.scala`). With dual-run on
in tests, `COUNT(t.*)` vs `count(t.*)` may behave differently across analyzers
unless fixed-point is updated in the same PR. Should we either fix both sides
together or keep a case-sensitive path for the table-star assertion only?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -409,7 +410,7 @@ object SQLConf {
)
.version("4.1.0")
.doubleConf
- .createWithDefault(if (Utils.isTesting) 1.0 else 0.001)
+ .createWithDefault(if (Utils.isTesting) 1.0 else 0.1)
Review Comment:
Do we know the effect of this change on CI runtimes? Are the latest CI runs
on this PR reflective of this change, such that we can compare their e2e
runtimes with those before the change and confirm they are reasonable?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/ResolverGuard.scala:
##########
@@ -392,11 +380,28 @@ class ResolverGuard(
* [[UnresolvedStarExceptOrReplace]] is handled separately, because it's a
leaf expression,
* but it has replacement expressions, that could be non-trivial.
*/
- private def checkStar(star: Star) = star match {
- case starExceptOrReplace: UnresolvedStarExceptOrReplace =>
- starExceptOrReplace.replacements.collectFirst { case
CheckExpressionSeq(reason) => reason }
- case star =>
- star.children.collectFirst { case CheckExpression(reason) => reason }
+ private def checkStar(star: Star) =
+ checkStarTarget(star).orElse {
+ star match {
+ case starExceptOrReplace: UnresolvedStarExceptOrReplace =>
+ starExceptOrReplace.replacements.collectFirst {
+ case CheckExpressionSeq(reason) => reason
+ }
+ case star =>
+ star.children.collectFirst { case CheckExpression(reason) => reason }
+ }
+ }
+
+ private def checkStarTarget(star: Star): Option[String] = star match {
Review Comment:
Here we are modifying `ResolverGuard` a lot; this PR:
- drops explicit lambda / grouping-sets expression checks
- rejects session-qualified UDFs outright
- treats generators and higher-order functions as unsupported by name
- adds `checkStarTarget` for metadata column stars
- gates `prioritizeOrdinalResolutionInSort=false` and
`persistentCatalogFirst=true`
That is conservative and appropriate for dual-run safety, but
`ResolverGuardSuite` is not in the changed files yet. Should we add unit tests
for this? I'm OK with changing the `ResolverGuard` in the same PR as enabling
the dual-runs here if it helps get us out the door on the dual run coverage.
--
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]
