sarutak opened a new pull request #31601:
URL: https://github.com/apache/spark/pull/31601
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section
is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster
reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class
hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other
DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR proposes to introduce a new syntax `attr()` to represent attributes
with the Catalyst DSL.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
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?
<!--
Note that it means *any* user-facing change including all aspects such as
the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes
- provide the console output, description and/or an example to show the
behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to
the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes. This PR introduces `attr()` for users to represent attributes but
doesn't break API compatibility.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some
test cases that check the changes thoroughly including negative and positive
cases if possible.
If it was tested in a way different from regular unit tests, please clarify
how you tested step by step, ideally copy and paste-able, so that other
reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why
it was difficult to add.
-->
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
