[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 @HyukjinKwon Thanks for looking into this. It is based on the comment and discussion here https://github.com/apache/spark/pull/21193#discussion_r186627099. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user HyukjinKwon commented on the issue: https://github.com/apache/spark/pull/21520 @viirya ~ I was just trying to read the PRs. Would you please mind if I ask where is the "Based on previous discussion" ? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 As I will incrementally split this into smaller PRs, I will first close this. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 @kiszk Seems good to me. I will go to create and use those APIs in pieces of PRs. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21520
For 1, I agree with you that it is not good to introduce many APIs at
first. On the other hand, it would be good to prepare only a few APIs that are
frequently used, not to prepare many APIs. It make code simpler and cleaner.
I think that `inline"${classOf[...].getName}"` and
`inline"${CodeGenerator.javaType(...)}"` are frequently used.
`inline"${CodeGenerator.boxedType(...)}"` may be prepared for consistency with
`inline"${CodeGenerator.javaType(...)}"`.
For 2, new APIs in `ctx` can co-exist with old APIs. Thus, to introduce new
APIs can co-exist with your new proposal.
WDYT?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 > 1. We are seeing many inline prefix with a few typical patterns. > Can we introduce new APIs to avoid repetations of adding inline, for example JavaCode.className(Class[_]): JavaCode for the first call. @kiszk I initially took a similar approach but found soon that I'd create too many APIs. I'm not pretty sure if that is what we want to have distinguish them in API level because they are all actually a simple piece of inline string in code, so I turned to a `inline` to treat them as same. > 2. We are seeing many JavaCode.global() or JavaCode.variable() when we create a new variable. Would it be possible to make them simpler? Yes, I noticed that too. I was planning to change existing API such as `ctx.freshName`. But I leave it as it and set the first goal to pass all tests after forbidding string interpolation. Since the tests are passed now, I think we can incrementally make the changes more simpler and clear. I've proposed to do this part in some smaller PRs (ref: https://github.com/apache/spark/pull/21520#issuecomment-396111725). WDYT? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 @kiszk @mgaido91 Thanks for your comment! > What do you think about starting doing the needed changes in smaller PRs which focus only on specific part and forbidding the string interpolation after those have made the needed changes smaller? By disallowing string interpolation in code blocks, any strings passed into a code block won't pass the compilation. It is also more guaranteed that we don't miss any strings. It is why this change is quite big and not in many smaller pieces. Most important is, I need to have all the changes together to see if we can pass all the tests once we completely forbid string interpolation. But it doesn't mean we need to review and merge this big change. It is still possible to break this into smaller PRs. It may work like this: 1. Split a part of change into a smaller PR, review it and finally merge it. 2. Incorporate the merged change back to this PR. Make test passed. Go back to step 1. WDYT? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user mgaido91 commented on the issue: https://github.com/apache/spark/pull/21520 yes, this is a great work @viirya ! It would be great if we can split it into smaller updates. What do you think about starting doing the needed changes in smaller PRs which focus only on specific part and forbidding the string interpolation after those have made the needed changes smaller? I also like and agree with @kiszk about introducing new APIs. In particular, in the last days I checked the part related to the usage of `freshName `. IMO, we can introduce a new method `addLocalVariable` (and maybe renaming `addMutableState` to `addGlobalVariable`) and make `freshName` private (or if it not feasible as we are using it also for methods' names we can restrict its usage only to that particular case). What do you think? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21520
Thank you for a lot of works to update many places. It is very hard to
split it into several pieces.
Now, we are seeing several typical patterns in the all of changes, in
paticular by wrapping the original code. Would it be possible to make changes
simpler by introducing several APIs?
1) We are seeing many `inline` prefix with a few typical patterns.
```
inline"${classOf[...].getName}"
inline"${CodeGenerator.javaType(...)}"
inline"${CodeGenerator.boxedType(...)}"
```
Can we introduce new APIs to avoid repetations of adding `inline`, for
example `JavaCode.className(Class[_]): JavaCode` for the first call.
2) We are seeing many `JavaCode.global()` or `JavaCode.variable()` when we
create a new variable.
Would it be possible to make them simpler?
For example, we may introduce these APIs.
```
ctx.addMutableState(..., Class[_])
ctx.freshName(..., DataType)
ctx.freshNameIsNull(...)
```
The first one calls `JavaCode.global()` in the method. The second one calls
`JavaCode.variable()`.
WDYT?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21520 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91631/ Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21520 Merged build finished. Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/21520 **[Test build #91631 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91631/testReport)** for PR 21520 at commit [`2a85388`](https://github.com/apache/spark/commit/2a85388276d7227d04c24110756236bc9da063d4). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21520 Merged build finished. Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/21520 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/3884/ Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/21520 **[Test build #91631 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91631/testReport)** for PR 21520 at commit [`2a85388`](https://github.com/apache/spark/commit/2a85388276d7227d04c24110756236bc9da063d4). --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 Sorry this change is quite large, but it can't split into smaller pieces because it must be changed as a whole to pass compilation. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #21520: [SPARK-24505][SQL] Forbidding string interpolation in Co...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/21520 cc @cloud-fan @kiszk @hvanhovell --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
