[GitHub] [spark] allisonwang-db commented on a diff in pull request #39691: [SPARK-31561][SQL] Add QUALIFY clause

["allisonwang-db (via GitHub)" <gi...@apache.org>]

allisonwang-db commented on code in PR #39691:
URL: https://github.com/apache/spark/pull/39691#discussion_r1129182842


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1729,6 +1729,23 @@ class Analyzer(override val catalogManager: CatalogManager)
           resolveExpressionByPlanChildren(resolvedWithAgg, u, allowOuter = true)
         }
 
+      case u @ UnresolvedQualify(cond, child) if !u.resolved && child.resolved =>
+        if (!u.containsPattern(WINDOW_EXPRESSION)) {

Review Comment:
   What if we have a subquery that contains a window function but not the main query: 
   ```
   SELECT (SELECT window_func w FROM t QUALIFY t.c1 > 0) FROM t QUALIFY t.c1 > 0
   ```



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -1729,6 +1729,23 @@ class Analyzer(override val catalogManager: CatalogManager)
           resolveExpressionByPlanChildren(resolvedWithAgg, u, allowOuter = true)
         }
 
+      case u @ UnresolvedQualify(cond, child) if !u.resolved && child.resolved =>
+        if (!u.containsPattern(WINDOW_EXPRESSION)) {
+          throw QueryCompilationErrors.expressionWithoutWindowExpressionError(cond)
+        } else {
+          if (u.missingInput.nonEmpty) {
+            val (newCond, newChild) = resolveExprsAndAddMissingAttrs(Seq(cond), child)

Review Comment:
   If we treat the resolution order for QUALIFY the same as GROUP BY, then we need to resolve the attributes in the QUALIFY clause first using the columns in the FROM clause and then using columns and aliases in the SELECT clause.
   
   For example: `SELECT c1, window_func AS c2 FROM t QUALIFY c2 > 0;`  here `c2` should be resolved as `t.c2` instead of the `window_func as c2`.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -571,7 +571,7 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase {
       messageParameters = Map.empty)
   }
 
-  def expressionWithoutWindowExpressionError(expr: NamedExpression): Throwable = {
+  def expressionWithoutWindowExpressionError(expr: Expression): Throwable = {

Review Comment:
   Can we use a new error for this? Currnetly the error message is not very user-friendly. E.g.:
   ```
   SELECT * FROM t QUALIFY c1 > 0 
   
   ('c1 > 0) does not have any WindowExpression.
   ```



##########
sql/core/src/test/resources/sql-tests/inputs/window.sql:
##########
@@ -465,3 +465,60 @@ SELECT
 	SUM(salary) OVER w sum_salary
 FROM
 	basic_pays;
+
+-- Test QUALIFY clause

Review Comment:
   We can split these tests into a new file.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org