Skip to content

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Feb 22, 2021

What changes were proposed in this pull request?

The current expression dsl API heavily relies on Symbol, which is deprecated in Scala 3. However, the string support in the expression dsl API is pretty weak.

This PR improves the dsl API to let "name".int return an int attribute, which is the same as 'name.int. The same to other types.

To do this, this PR removes the string literal implicit, which is not widely used, and adds a new string implicit that creates attributes, which is the same as the symbol implicit. As a consequence, "abc" like ... now creates a Like expression with attribute abc, not string literal "abc". To use literal, this PR adds a new way: "abc".lit like ...

Why are the changes needed?

To slowly retire Symbol.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Comment out DslString and compile, and see which places rely on string implicits. Also check failed tests.

Existing tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now "2018-03-18".date returns a date type attribute, so I updated this test to use a method.

@cloud-fan
Copy link
Contributor Author

@sarutak how about this?

}
implicit class DslAttr(attr: UnresolvedAttribute) extends ImplicitAttribute {
def s: String = attr.name
implicit class DslString(str: String) extends ImplicitAttribute {
Copy link
Member

Choose a reason for hiding this comment

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

Why not s: String ?

@sarutak
Copy link
Member

sarutak commented Feb 22, 2021

Hmm, it seems to break the usage of string literals to represent Literal though it doesn't affect end user as it's internal...
I wanted to avoid breaking the usage so I proposed different way attr"" with string interpolation in #31601 .
But if others think it's reasonable, It's OK.

@SparkQA
Copy link

SparkQA commented Feb 22, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39933/

@SparkQA
Copy link

SparkQA commented Feb 22, 2021

Test build #135353 has finished for PR 31612 at commit 48ef380.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • implicit class DslString(str: String) extends ImplicitAttribute

@SparkQA
Copy link

SparkQA commented Feb 22, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39933/

@cloud-fan
Copy link
Contributor Author

It's internal so I think we can update the DSL API to the best shape we want. However, it does raise some issues after I looking into the test failures:

where(col === "abc")
select("abc".as(...))
"abc" like ...

Previously they all create expressions with string literal, and breaking them seems a much bigger impact than I thought. I tend to agree that we should create a new syntax for creating attributes from strings.

@cloud-fan cloud-fan closed this Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants