Skip to content

[SPARK-25769][SQL]make UnresolvedAttribute.sql escape nested columns correctly #22788

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
huaxingao wants to merge 9 commits into from
Closed

[SPARK-25769][SQL]make UnresolvedAttribute.sql escape nested columns correctly #22788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2aa4ad9
[SPARK-25769][SQL]change nested columns from to .
huaxingao Oct 21, 2018
e941acd
address comment
huaxingao Oct 21, 2018
99bfd00
add test
huaxingao Oct 22, 2018
458f77a
move change to UnresolvedAttribute.sql
huaxingao Oct 25, 2018
566d5f9
Refactor the test case
dongjoon-hyun Oct 28, 2018
7ff2696
Merge pull request #1 from dongjoon-hyun/PR-22788
huaxingao Oct 30, 2018
693c512
make UnresolvedAttribute the same as UnresolvedAttribute.name
huaxingao Nov 1, 2018
05688f5
change test case name
huaxingao Nov 1, 2018
3c81840
fix test faliure
huaxingao Nov 2, 2018
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
14 changes: 12 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,18 @@ case class UnresolvedTableValuedFunction(
*/
case class UnresolvedAttribute(nameParts: Seq[String]) extends Attribute with Unevaluable {

def name: String =
nameParts.map(n => if (n.contains(".")) s"`$n`" else n).mkString(".")
def name: String = {
nameParts.map(n =>
if (n.contains(".")) s"`$n`"
else {
if (nameParts.length > 1) {
s"`$n`"
} else {
n
}
}
).mkString(".")
}
Copy link
Member

@dongjoon-hyun dongjoon-hyun Oct 21, 2018

Choose a reason for hiding this comment

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

Hi, @huaxingao . Can we have the following one-line patch instead 12 line changes?

- nameParts.map(n => if (n.contains(".")) s"`$n`" else n).mkString(".")
+ nameParts.map(n => if (nameParts.length > 1 || n.contains(".")) s"`$n`" else n).mkString(".")

cc @cloud-fan and @gatorsmile and @dbtsai .

Copy link
Contributor Author

@huaxingao huaxingao Oct 21, 2018

Choose a reason for hiding this comment

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

@dongjoon-hyun Thanks! Fixed.


override def exprId: ExprId = throw new UnresolvedException(this, "exprId")
override def dataType: DataType = throw new UnresolvedException(this, "dataType")
Expand Down
16 changes: 8 additions & 8 deletions sql/core/src/test/resources/sql-tests/results/columnresolution-negative.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ SELECT t1.i1 FROM t1, mydb1.t1
struct<>
-- !query 9 output
org.apache.spark.sql.AnalysisException
Reference 't1.i1' is ambiguous, could be: mydb1.t1.i1, mydb1.t1.i1.; line 1 pos 7
Reference '`t1`.`i1`' is ambiguous, could be: mydb1.t1.i1, mydb1.t1.i1.; line 1 pos 7
Copy link
Contributor

@cloud-fan cloud-fan Oct 22, 2018

Choose a reason for hiding this comment

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

why is the new format better? it's more verbose, isn't it?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Oct 22, 2018

Choose a reason for hiding this comment

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

I thought some examples in the above.

Copy link
Contributor

@cloud-fan cloud-fan Oct 23, 2018

Choose a reason for hiding this comment

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

These examples only make sense when we have the outer backticks. e.g. 't1.i1' is good.



-- !query 10
Expand All @@ -90,7 +90,7 @@ SELECT mydb1.t1.i1 FROM t1, mydb1.t1
struct<>
-- !query 10 output
org.apache.spark.sql.AnalysisException
Reference 'mydb1.t1.i1' is ambiguous, could be: mydb1.t1.i1, mydb1.t1.i1.; line 1 pos 7
Reference '`mydb1`.`t1`.`i1`' is ambiguous, could be: mydb1.t1.i1, mydb1.t1.i1.; line 1 pos 7


-- !query 11
Expand All @@ -108,7 +108,7 @@ SELECT t1.i1 FROM t1, mydb2.t1
struct<>
-- !query 12 output
org.apache.spark.sql.AnalysisException
Reference 't1.i1' is ambiguous, could be: mydb1.t1.i1, mydb2.t1.i1.; line 1 pos 7
Reference '`t1`.`i1`' is ambiguous, could be: mydb1.t1.i1, mydb2.t1.i1.; line 1 pos 7


-- !query 13
Expand All @@ -134,7 +134,7 @@ SELECT t1.i1 FROM t1, mydb1.t1
struct<>
-- !query 15 output
org.apache.spark.sql.AnalysisException
Reference 't1.i1' is ambiguous, could be: mydb2.t1.i1, mydb1.t1.i1.; line 1 pos 7
Reference '`t1`.`i1`' is ambiguous, could be: mydb2.t1.i1, mydb1.t1.i1.; line 1 pos 7


-- !query 16
Expand All @@ -152,7 +152,7 @@ SELECT t1.i1 FROM t1, mydb2.t1
struct<>
-- !query 17 output
org.apache.spark.sql.AnalysisException
Reference 't1.i1' is ambiguous, could be: mydb2.t1.i1, mydb2.t1.i1.; line 1 pos 7
Reference '`t1`.`i1`' is ambiguous, could be: mydb2.t1.i1, mydb2.t1.i1.; line 1 pos 7


-- !query 18
Expand All @@ -161,7 +161,7 @@ SELECT db1.t1.i1 FROM t1, mydb2.t1
struct<>
-- !query 18 output
org.apache.spark.sql.AnalysisException
cannot resolve '`db1.t1.i1`' given input columns: [mydb2.t1.i1, mydb2.t1.i1]; line 1 pos 7
cannot resolve '`db1`.`t1`.`i1`' given input columns: [mydb2.t1.i1, mydb2.t1.i1]; line 1 pos 7
Copy link
Contributor

@cloud-fan cloud-fan Oct 29, 2018

Choose a reason for hiding this comment

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

do you think we should just make sql same as name? It looks to me that 'db1.t1.i1' is better than '`db1`.`t1`.`i1`', as it's more compact and is not ambiguous.

Copy link
Contributor Author

@huaxingao huaxingao Oct 30, 2018

Choose a reason for hiding this comment

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

Yes. I agree. For the four examples, we will have the following results:

$"a.b".expr.asInstanceOf[org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute].sql

with my previous fix:

`a`.`b`

make sql same as name:

a.b

$"`a.b`".expr.asInstanceOf[org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute].sql

with my previous fix:

`a`.`b`

make sql same as name:

`a.b`

$"`a`.b".expr.asInstanceOf[org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute].sql

with my previous fix:

`a`.`b`

make sql same as name:

a.b

$"`a.b`.c".expr.asInstanceOf[org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute].sql

with my previous fix:

`a.b`.`c`

make sql same as name:

`a.b`.c

Does this look good to everybody?

Copy link
Contributor

@cloud-fan cloud-fan Oct 31, 2018

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dilipbiswal dilipbiswal Oct 31, 2018

Choose a reason for hiding this comment

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

@huaxingao Were you planning to make a change to make name same as sql ? Currently they are different ?

Copy link
Contributor

@dilipbiswal dilipbiswal Oct 31, 2018

Choose a reason for hiding this comment

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

@cloud-fan I had a question, the 3rd example from huaxin, wanted to confirm again if the output looks okay to you.

from huaxin

$"`a`.b".expr.asInstanceOf[org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute].sql

with my previous fix:

`a`.`b`

make sql same as name:

a.b

Is it okay to drop the backtick from the first identifier ?

Copy link
Contributor Author

@huaxingao huaxingao Oct 31, 2018

Choose a reason for hiding this comment

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

@dilipbiswal
Currently sql is not the same as name

Copy link
Contributor

@cloud-fan cloud-fan Oct 31, 2018

Choose a reason for hiding this comment

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

Is it okay to drop the backtick from the first identifier

AFAIK both name and sql are for display/message. I think dropping backtick is fine if no ambiguous

Copy link
Contributor

@dilipbiswal dilipbiswal Oct 31, 2018

Choose a reason for hiding this comment

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

@cloud-fan Makes sense. Thanks for clarification...



-- !query 19
Expand All @@ -186,7 +186,7 @@ SELECT mydb1.t1 FROM t1
struct<>
-- !query 21 output
org.apache.spark.sql.AnalysisException
cannot resolve '`mydb1.t1`' given input columns: [mydb1.t1.i1]; line 1 pos 7
cannot resolve '`mydb1`.`t1`' given input columns: [mydb1.t1.i1]; line 1 pos 7


-- !query 22
Expand Down Expand Up @@ -221,7 +221,7 @@ SELECT mydb1.t1.i1 FROM t1
struct<>
-- !query 25 output
org.apache.spark.sql.AnalysisException
cannot resolve '`mydb1.t1.i1`' given input columns: [mydb2.t1.i1]; line 1 pos 7
cannot resolve '`mydb1`.`t1`.`i1`' given input columns: [mydb2.t1.i1]; line 1 pos 7


-- !query 26
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/resources/sql-tests/results/describe-table-column.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ DESC FORMATTED desc_complex_col_table col.x
struct<>
-- !query 14 output
org.apache.spark.sql.AnalysisException
DESC TABLE COLUMN command does not support nested data types: col.x;
DESC TABLE COLUMN command does not support nested data types: `col`.`x`;


-- !query 15
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/resources/sql-tests/results/query_regex_column.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ SELECT t.`(a)?+.+` FROM testData2 t WHERE a = 1
struct<>
-- !query 4 output
org.apache.spark.sql.AnalysisException
cannot resolve 't.`(a)?+.+`' given input columns: [t.A, t.B, t.c, t.d]; line 1 pos 7
cannot resolve '`t`.`(a)?+.+`' given input columns: [t.A, t.B, t.c, t.d]; line 1 pos 7


-- !query 5
Expand Down
4 changes: 2 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,7 +1143,7 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
|order by struct.a, struct.b
|""".stripMargin)
}
assert(error.message contains "cannot resolve '`struct.a`' given input columns: [a, b]")
assert(error.message contains "cannot resolve '`struct`.`a`' given input columns: [a, b]")

}

Expand Down Expand Up @@ -2702,7 +2702,7 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {

val e = intercept[AnalysisException](sql("SELECT v.i from (SELECT i FROM v)"))
assert(e.message ==
"cannot resolve '`v.i`' given input columns: [__auto_generated_subquery_name.i]")
Copy link
Contributor

@cloud-fan cloud-fan Oct 22, 2018

Choose a reason for hiding this comment

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

I think the problem here is the out-most backticks, do you know where we add it?

Copy link
Contributor Author

@huaxingao huaxingao Oct 22, 2018

Choose a reason for hiding this comment

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

The out-most backticks is added in
case _, in case class UnresolvedAttribute(nameParts: Seq[String])

  override def sql: String = name match {
    case ParserUtils.escapedIdentifier(_) | ParserUtils.qualifiedEscapedIdentifier(_, _) => name
    case _ => quoteIdentifier(name)
  }

the nameParts is a Seq of "v" "i", name is v.i

"cannot resolve '`v`.`i`' given input columns: [__auto_generated_subquery_name.i]")

checkAnswer(sql("SELECT __auto_generated_subquery_name.i from (SELECT i FROM v)"), Row(1))
}
Expand Down