You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "rshkv (via GitHub)" <gi...@apache.org> on 2023/04/21 20:03:13 UTC
[GitHub] [spark] rshkv opened a new pull request, #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
rshkv opened a new pull request, #40902:
URL: https://github.com/apache/spark/pull/40902
Re-attempting #40794. #40794 tried to more safely create `AttributeReference` objects from multi-part attributes in `ImplicitAttribute`. But that broke things and we had to revert. This PR is limiting the fix to the `UnresolvedAttribute` object returned by `DslAttr.attr`, which is enough to fix the issue here.
### What changes were proposed in this pull request?
This PR fixes DSL expressions on attributes with special characters by making `DslAttr.attr` and `DslAttr.expr` return the implicitly wrapped attribute instead of creating a new one.
### Why are the changes needed?
SPARK-43142: DSL expressions on attributes with special characters don't work even if the attribute names are quoted:
```scala
scala> "`slashed/col`".attr
res0: org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute = 'slashed/col
scala> "`slashed/col`".attr.asc
org.apache.spark.sql.catalyst.parser.ParseException:
mismatched input '/' expecting {<EOF>, '.', '-'}(line 1, pos 7)
== SQL ==
slashed/col
-------^^^
```
DSL expressions rely on a call to `expr` to get child of the new expression [(e.g.)](https://github.com/apache/spark/blob/87a5442f7ed96b11051d8a9333476d080054e5a0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala#L149).
`expr` here is a call on implicit class `DslAttr` that's wrapping the `UnresolvedAttribute` returned by `"...".attr` is wrapped by the implicit class `DslAttr`.
`DslAttr` and its super class implement `DslAttr.expr` such that a new `UnresolvedAttribute` is created from `UnresolvedAttribute.name` of the wrapped attribute [(here)](https://github.com/apache/spark/blob/87a5442f7ed96b11051d8a9333476d080054e5a0/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala#L273-L280).
But `UnresolvedAttribute.name` drops the quotes and thus the newly created `UnresolvedAttribute` parses an identifier that should be quoted but isn't:
```scala
scala> "`col/slash`".attr.name
res5: String = col/slash
```
### Does this PR introduce _any_ user-facing change?
DSL expressions on attributes with special characters no longer fail.
### How was this patch tested?
I couldn't find a suite testing the implicit classes in the DSL package, but the DSL package seems used widely enough that I'm confident this doesn't break existing behavior.
Locally, I was able to reproduce with this test; it was failing before and passes now:
```scala
test("chained DSL expressions on attributes with special characters") {
$"`slashed/col`".asc
}
```
--
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
[GitHub] [spark] rshkv commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1175069446
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -272,7 +272,7 @@ package object dsl {
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
}
implicit class DslAttr(attr: UnresolvedAttribute) extends ImplicitAttribute {
Review Comment:
Yes, done! 51d838e
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174341468
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -271,8 +271,9 @@ package object dsl {
override def expr: Expression = Literal(s)
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
}
- implicit class DslAttr(attr: UnresolvedAttribute) extends ImplicitAttribute {
- def s: String = attr.name
+ implicit class DslAttr(a: UnresolvedAttribute) extends ImplicitAttribute {
+ def s: String = a.name
+ override def attr: UnresolvedAttribute = a
Review Comment:
let's fix this completely so that people won't step into this pitfall again.
first problem:
```
implicit class DslAttr(a: UnresolvedAttribute) ... {
def s: String = a.name
}
abstract class ImplicitAttribute ... {
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
}
```
I think the real problem is `UnresolvedAttribute.name` can't be parsed back so this roundtrip is broken. We should update `def s` to
```
def s: String = a.sql
```
The second problem:
```
implicit class DslAttr(a: UnresolvedAttribute) ... {
def s: String = a.name
}
abstract class ImplicitAttribute ... {
def int: AttributeReference = AttributeReference(s, IntegerType, nullable = true)()
}
```
The `s` is the qualified column name, and we should handle it properly when creating `AttributeReference`
```
def int: AttributeReference = AttributeReference(attr.nameParts.head, IntegerType, nullable = true)(qualifier = attr.nameParts.init)
```
--
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
[GitHub] [spark] rshkv commented on pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on PR #40902:
URL: https://github.com/apache/spark/pull/40902#issuecomment-1521504998
@cloud-fan, updated for your suggestions. Tests ran and are green.
--
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
[GitHub] [spark] rshkv commented on pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on PR #40902:
URL: https://github.com/apache/spark/pull/40902#issuecomment-1519025734
@cloud-fan, tests are passing. Let me know what you think.
--
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
[GitHub] [spark] rshkv commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1175068610
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -281,89 +281,108 @@ package object dsl {
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
/** Creates a new AttributeReference of type boolean */
- def boolean: AttributeReference = AttributeReference(s, BooleanType, nullable = true)()
+ def boolean: AttributeReference =
+ AttributeReference(attr.nameParts.last, BooleanType)(qualifier = attr.nameParts.init)
Review Comment:
Yes! 7b50a886
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174777991
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -272,7 +272,7 @@ package object dsl {
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
}
implicit class DslAttr(attr: UnresolvedAttribute) extends ImplicitAttribute {
Review Comment:
shall we add `override` to save a roundtrip?
--
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
[GitHub] [spark] cloud-fan closed pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
URL: https://github.com/apache/spark/pull/40902
--
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
[GitHub] [spark] rshkv commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174400261
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategySuite.scala:
##########
@@ -27,18 +27,18 @@ import org.apache.spark.sql.types.{IntegerType, StringType, StructField, StructT
class DataSourceStrategySuite extends PlanTest with SharedSparkSession {
val attrInts = Seq(
$"cint".int,
- $"c.int".int,
+ $"`c.int`".int,
Review Comment:
The AttributeReferences created from these were previously treated as single-part which is why I'm updating the test to actually make these single-part (according to the comments on the test that's the expected behaviour here).
We could decide to treat `$"...".int` as single-part instead, which maybe is less of a change compared to the current behavior. But other tests rely on producing multi-part identifiers with `$"left.a"` [(here)](https://github.com/apache/spark/blob/87a5442f7ed96b11051d8a9333476d080054e5a0/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceOperatorSuite.scala#L180).
--
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
[GitHub] [spark] rshkv commented on pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on PR #40902:
URL: https://github.com/apache/spark/pull/40902#issuecomment-1521689588
Thank you!
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174777097
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -281,89 +281,108 @@ package object dsl {
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
/** Creates a new AttributeReference of type boolean */
- def boolean: AttributeReference = AttributeReference(s, BooleanType, nullable = true)()
+ def boolean: AttributeReference =
+ AttributeReference(attr.nameParts.last, BooleanType)(qualifier = attr.nameParts.init)
Review Comment:
can add a new method to create attribute to deduplicate code? The only difference is the data type.
--
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
[GitHub] [spark] cloud-fan commented on pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40902:
URL: https://github.com/apache/spark/pull/40902#issuecomment-1521544389
thanks, merging to master!
--
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
[GitHub] [spark] cloud-fan commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174341668
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -271,8 +271,9 @@ package object dsl {
override def expr: Expression = Literal(s)
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
}
- implicit class DslAttr(attr: UnresolvedAttribute) extends ImplicitAttribute {
- def s: String = attr.name
+ implicit class DslAttr(a: UnresolvedAttribute) extends ImplicitAttribute {
+ def s: String = a.name
+ override def attr: UnresolvedAttribute = a
Review Comment:
overriding `attr` in `DslAttr` is just an optimization to avoid the roundtrip, but it does not fix the problem, just hides it.
--
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
[GitHub] [spark] rshkv commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174398567
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSQLBuilderSuite.scala:
##########
@@ -95,7 +95,7 @@ class ExpressionSQLBuilderSuite extends SparkFunSuite {
test("attributes") {
checkSQL($"a".int, "a")
- checkSQL(Symbol("foo bar").int, "`foo bar`")
+ checkSQL(Symbol("`foo bar`").int, "`foo bar`")
Review Comment:
`Symbol("foo bar").int` previously just passed `"foo bar"` to the `AttributeReference` without parsing an `UnresolvedAttribute`. We now call `attr()` which _is_ parsing `"foo bar"` and that fails without backticks.
Alternatively, we can `quoteIfNeeded` in `DslSymbol` and treat, e.g., `Symbol("foo.bar")` as single-part (which is the current behaviour). But I think it's confusing if `$"foo.bar"` returns a multi-part attribute and `Symbol("foo.bar")` doesn't.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionSQLBuilderSuite.scala:
##########
@@ -95,7 +95,7 @@ class ExpressionSQLBuilderSuite extends SparkFunSuite {
test("attributes") {
checkSQL($"a".int, "a")
- checkSQL(Symbol("foo bar").int, "`foo bar`")
+ checkSQL(Symbol("`foo bar`").int, "`foo bar`")
Review Comment:
`Symbol("foo bar").int` previously just passed `"foo bar"` to the `AttributeReference` without parsing an `UnresolvedAttribute`. We now call `attr()` which _is_ parsing `"foo bar"` and that fails without backticks.
Alternatively, we can `quoteIfNeeded` in `DslSymbol` and treat, e.g., `Symbol("foo.bar")` as single-part (which is the current behaviour). But I think it's confusing if `$"foo.bar"` returns a multi-part attribute and `Symbol("foo.bar")` doesn't.
So I rather just update the test.
--
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
[GitHub] [spark] rshkv commented on a diff in pull request #40902: [SPARK-43142] Fix DSL expressions on attributes with special characters
Posted by "rshkv (via GitHub)" <gi...@apache.org>.
rshkv commented on code in PR #40902:
URL: https://github.com/apache/spark/pull/40902#discussion_r1174402747
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/dsl/package.scala:
##########
@@ -271,8 +271,9 @@ package object dsl {
override def expr: Expression = Literal(s)
def attr: UnresolvedAttribute = analysis.UnresolvedAttribute(s)
}
- implicit class DslAttr(attr: UnresolvedAttribute) extends ImplicitAttribute {
- def s: String = attr.name
+ implicit class DslAttr(a: UnresolvedAttribute) extends ImplicitAttribute {
+ def s: String = a.name
+ override def attr: UnresolvedAttribute = a
Review Comment:
Yeah, this seems worth fixing.
Some tests have been relying on multi-part attributes implicitly becoming single-part references. And we have to update those tests now to correct the behavior for multi-part attributes. I don't know if you consider that a wider break. It seems the DSL is solely used for testing, but technically these are public classes.
--
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