[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/19938 Sure, in #19865, I will generate new parameter name in `splitExpression` instead of inserting assertion. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/19938 I prefer to generate new parameter name in `splitExpression` over localizing global variables. There is no contract that an `Expression` must output java variables, we may inline some values, e.g. we already output `false` or `true` literal for `isNull`, and I roughly remember we do the same thing for `value`, e.g. `a + 1` instead of a new java variable `c` which is calculated by `c = a + 1`. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user maropu commented on the issue: https://github.com/apache/spark/pull/19938 Is there only the place where we need this localization (I mean other operators don't need this logic)? I'm also neutral about this pr though, I feel better to make this more general to avoid the same situation in the other existing (and new) operators. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/19938 This makes sense to me. Currently, based on previous discussion, we are fixing code generation and insert an assertion at #19865 to ensure no global variables are passed. Of course, as @cloud-fan said, generating a new parameter name for global variable. Which one is better solution? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/19938 Good point. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/19938 actually there is one real problem: after we fold many global variables into an array, the variable name may become something like `arr[1]`, which can't be used as the parameter name. Localize the global variables in current expression/operator is one solution, another one is generating parameter names instead of reusing the input variable name. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user cloud-fan commented on the issue: https://github.com/apache/spark/pull/19938 I have the same feeling with @viirya , expressions/operators usually won't mutate input from children. One concern is how to guarantee this programmatically, but might be OK as I don't think this would happen. We can also remove the temp global variables introduced by the recent commits from @kiszk --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/19938 Thank you for great thought. Let me think about it. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/19938 They are possibly passed in split functions. But it is hard to image a case we will change their values in the functions. In SparkSQL, the output from a child operator are just used as input to evaluate new output in the parent operator. We don't use the output as mutable statuses. The possible problematic case is when we create a global variable in an operator/expression codegen and use this global variable to carry mutable status (e.g., the condition meeting status in casewhen) during the evaluation of the op/expr. Then it is possibly we pass it and modify it in split functions. If we don't change the values, seems to me this change just creates redundant local variables. This doesn't cause any harm at all. So I feel no strong option for this. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/19938 Can we guarantee 100% that any operation in `consume()` at [here](https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala#L255) does not pass global variables into split functions? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user viirya commented on the issue: https://github.com/apache/spark/pull/19938 IIUC, the problem is only happened when we wrongly pass global variables into split functions and change the values. Will we change the result variables from aggregation? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/19938 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 #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/19938 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84694/ Test PASSed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/19938 **[Test build #84694 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84694/testReport)** for PR 19938 at commit [`ac65dd2`](https://github.com/apache/spark/commit/ac65dd21b975481f5c4feb5f0745a2c45d000728). * 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 #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/19938 **[Test build #84694 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84694/testReport)** for PR 19938 at commit [`ac65dd2`](https://github.com/apache/spark/commit/ac65dd21b975481f5c4feb5f0745a2c45d000728). --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user kiszk commented on the issue: https://github.com/apache/spark/pull/19938 Jenkins, retest this please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/19938 Merged build finished. Test FAILed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user AmplabJenkins commented on the issue: https://github.com/apache/spark/pull/19938 Test FAILed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84691/ Test FAILed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/19938 **[Test build #84691 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84691/testReport)** for PR 19938 at commit [`ac65dd2`](https://github.com/apache/spark/commit/ac65dd21b975481f5c4feb5f0745a2c45d000728). * This patch **fails due to an unknown error code, -9**. * 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 #19938: [SPARK-22747][SQL] Localize lifetime of mutable states i...
Github user SparkQA commented on the issue: https://github.com/apache/spark/pull/19938 **[Test build #84691 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84691/testReport)** for PR 19938 at commit [`ac65dd2`](https://github.com/apache/spark/commit/ac65dd21b975481f5c4feb5f0745a2c45d000728). --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
