Skip to content

[SPARK-32347][SQL] Hint in CTE should be resolved in Hints batch rule #29156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TJX2014 wants to merge 2 commits into from
Closed

[SPARK-32347][SQL] Hint in CTE should be resolved in Hints batch rule #29156

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a20c29d
bug fix init
TJX2014 Jul 19, 2020
be0711a
style fix
TJX2014 Jul 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveHints.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@ package org.apache.spark.sql.catalyst.analysis
import java.util.Locale

import scala.collection.mutable

import org.apache.spark.sql.AnalysisException
import org.apache.spark.sql.catalyst.expressions.{Ascending, Expression, IntegerLiteral, SortOrder}
import org.apache.spark.sql.catalyst.expressions.{Ascending, Expression, IntegerLiteral, SortOrder, SubqueryExpression}
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.catalyst.trees.CurrentOrigin
Expand Down Expand Up @@ -165,6 +164,24 @@ object ResolveHints {
hintErrorHandler.hintRelationsNotFound(h.name, h.parameters, unmatchedIdents)
applied
}
case With(child, relations) => resolveCTEHint(child,
Copy link
Contributor

@LantaoJin LantaoJin Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch looks like it is specifically designed to fix With comparing to #29062. I am open to more comments.

relations.foldLeft(Seq.empty[(String, LogicalPlan)]) {
case (resolved, (name, relation)) =>
resolved :+ name -> apply(resolveCTEHint(relation, resolved))
})
}

def resolveCTEHint(plan: LogicalPlan, cteRelations: Seq[(String, LogicalPlan)]): LogicalPlan = {
plan resolveOperatorsDown {
case u: UnresolvedRelation =>
cteRelations.find(x => resolver(x._1, u.tableName)).map(_._2).getOrElse(u)
Comment on lines +177 to +178
Copy link
Contributor Author

@TJX2014 TJX2014 Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch will occur stackoverflow when cte table name is same as table in cte as follows:
sql("create temporary view t as select 1 as id")
sql("with t as (select /*+ BROADCAST(id) */ id from t) select id from t")
@cloud-fan Could you please help me find a way to pass this ?

Copy link
Contributor

@cloud-fan cloud-fan Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you looked at #29062 ? Seems easier to just run CTE substitution in the very beginning.

Copy link
Contributor Author

@TJX2014 TJX2014 Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, put the CTE substitution in the very beginning is an easier way.

case other =>
// This cannot be done in ResolveSubquery because ResolveSubquery does not know the CTE.
other transformExpressions {
case e: SubqueryExpression =>
e.withNewPlan(resolveCTEHint(e.plan, cteRelations))
}
}
}
}

Expand Down
7 changes: 7 additions & 0 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2558,6 +2558,13 @@ abstract class SQLQuerySuiteBase extends QueryTest with SQLTestUtils with TestHi
}
}
}

test("SPARK-32347: cte hint regression") {
withTempView("t") {
sql("create temporary view t as select 1 as id")
sql("with cte as (select /*+ BROADCAST(id) */ id from t) select id from cte")
Copy link
Member

@maropu maropu Jul 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case, could you check that the hist is correctly applied?

Copy link
Contributor Author

@TJX2014 TJX2014 Jul 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I need to check the hist, seems something wrong with the patch.

}
}
}

@SlowHiveTest
Expand Down