You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/02/20 15:10:45 UTC

[GitHub] [spark] sarutak opened a new pull request #31601: [SPARK-34484][SQL] Introduce a new syntax to represent attributes with the Catalyst DSL

sarutak opened a new pull request #31601:
URL: https://github.com/apache/spark/pull/31601

### What changes were proposed in this pull request?

This PR proposes to introduce a new syntax `attr()` to represent attributes with the Catalyst DSL.

### Why are the changes needed?

With the Catalyst DSL (`dsl/package.scala`), we have two ways to represent attributes.

1. Symbol literals (`'` syntax)
2. `$""` syntax which is defined in `sql/catalyst` module using string context.

But they have problems.

Regarding symbol literals, the scala community deprecates the symbol literals in Scala 2.13. We could alternatively use `Symbol` constructor but what is worse, Scala will completely remove `Symbol` in the future (https://scalacenter.github.io/scala-3-migration-guide/docs/incompatibilities/dropped-features.html).

> Although scala.Symbol is useful for migration, beware that it is deprecated and that it will be removed from the scala-library. You are recommended, as a second step, to replace them with plain string literals "xwy" or a dedicated class.

Regarding `$""` syntax, this has two problems.
The first problem is that the syntax conflicts with another `$""` syntax defined in `sql/core` module.
You can easily see the problem with the Spark Shell.

```
import org.apache.spark.sql.catalyst.dsl.expressions._
val attr1 = $"attr1"

error: type mismatch;
found : StringContext
required: ?{def $: ?}
Note that implicit conversions are not applicable because they are ambiguous:
both method StringToColumn in class SQLImplicits of type (sc: StringContext): spark.implicits.StringToColumn
and method StringToAttributeConversionHelper in trait ExpressionConversions of type (sc: StringContext): org.apache.spark.sql.catalyst.dsl.expressions.StringToAttributeConversionHelper
are possible conversion functions from StringContext to ?{def $: ?}

```

The second problem is that we can't write like `$"attr".map(StringType, StringType)`, though we can write `'attr.map(StringType, StringType)`.
This seems to be a bug of the Scala compiler and will be fixed in neither `2.12` nor `2.13` (https://github.com/scala/scala/pull/7396).

Actually, I'm working on replacing all the symbol literals with `$""` syntax in SPARK-34443 (#31569) and I found this problem in the following test code.

* EncoderResolutionSuite.scala
* ComplexTypeSuite.scala
* ObjectExpressionsSuite.scala
* NestedColumnAliasingSuite.scala
* ReplaceNullWithFalseInPredicateSuite.scala
* SimplifyCastsSuite.scala
* SimplifyConditionalSuite.scala

```
[error] /home/kou/work/oss/spark-scala-2.13/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/EncoderResolutionSuite.scala:212:28: too many arguments (found 2, expected 1) for method map: (f: org.apache.spark.sql.catalyst.expressions.Expression => A): Seq[A]
[error] $"a".map(StringType, StringType)).foreach { attr =>
```

As a solution for those problems, I propose to introduce a new syntax `attr("attr_name")`, which doesn't break the API compatibility and has the same capability as the symbol literal syntax.

Another solution would be using string interpolation but it's difficult to have workaround when the syntax is conflict with another string interpolation like `$""` defined in `catalyst` and `core` so I don't adopt this solution.

### Does this PR introduce _any_ user-facing change?

Yes. This PR introduces `attr()` for users to represent attributes but doesn't break API compatibility.

### How was this patch tested?

Modified the tests mentioned above to use `attr()` rather than symbol literals.

----------------------------------------------------------------
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.

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