Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
@cloud-fan Since #20043 was merged now, I will go to polish this and
implement the above idea. Will submit a PR for this when it is ready.
---
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/19813
LGTM, great work!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19813
Cool. LGTM, too.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
This is a pretty cool idea that can work with the current string based
codegen framework, LGTM!
---
-
To unsubscribe, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
I agree that the best is we can have both of them.
I have a proposal to replace statement output in split methods. Maybe you
can check if it sounds good.
By #20043, we have a
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
Do we have to sacrifice one of them? If we do then I agree deeply nested
expression is more common than a long chain of arithmetic expressions and we
should get this patch. I think we should
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
Which one is more common? A chain of arithmetic expressions? Or a deeply
nested expression? I don't see strong evidence that supports statement output
from the discussion. The only one possibility
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
I think all arithmetic, predicate and bitwise expressions can benefit from
it, and they are very common expressions in SQL. More importantly, allowing
expressions to output statement may have
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
This is only valid when by coincidence the all expressions involved can use
statement as output. As I looked at the codebase, I think only few expressions
can output statement. This may not apply
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
I did a search but can't find one in the current codebase, but I do think
this is a valid idea, e.g. a simple example would be `a + b + z`, if
expressions can output statement, then we just
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19813
IMHO, in general, the output `ev.value` would be declared as local variable
by parent as
```
s"""${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
```
Such as
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
Not all of expressions can use statement as output. Seems to me there are
only very few scenarios (e.g., a chain of expressions that all can use
statement output by coincidence) we can really save
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
> it also means the chained evaluation of expressions needs to be run at
every occurrence.
We can introduce some mechanism to save statement to local variables if
it's going to be
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
> What about future? Will we need to output statement for some reason? like
reducing the usage of local variables?
I think that we won't have strong motivation to use output statement. The
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
To me whole stage codegen compilation fix is less important as we can
fallback to non whole stage codegen, so we don't need to rush.
> As we don't use such statement as codegen output, I
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19813
I think that this PR is necessary to fix SPARK-22868 and SPARK-22869
---
-
To unsubscribe, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
@kiszk Yea, I have now checked through the codegen. I didn't find the
places that can cause that issue (`a + 1` as the codegen output value) yet. I
may submit another PR to let us easily identify
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19813
I found a few problems that this PR can ideally solve. If this is not
available soon, I will use workaround in upcoming PRs.
---
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19813
I agree that string replacement is too dangerous (e.g. `a + 1 = a + 10`
with `a + 1`).
How about a contract with adding assertions?
---
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
AST based codegen framework sounds a too far step from current point. I
think we either follow the new contract or refactor the current framework a
little bit.
---
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
replacing them via string is too dangerous, logically we wanna replace some
nodes in AST, which needs an AST based codegen framework, or we need to
refactor the current framework a little bit to
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
> Another solution is to not make this contract. By a quick look this seems
hard to do, because at the time of doing this, the code(method body) is already
generated and we don't know how to replace
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
@mgaido91 Thanks for the comment. I agreed that to make the contract is the
easiest way. If we don't make this contract, seems to me a significant change
is needed.
---
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19813
I am sorry, but I cannot give a precise answer. I can't think of any case
where this can happen. There are many places where we assume that what we get
is a literal or a variable name. But there
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
Note: One possible problem is in `a + 1`, `a` is not an input variable from
an operator.
---
-
To unsubscribe, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
I'm wondering if an output statement like `a + 1` is a problem.
If we only include codegen output with valid java variable names, an output
like `a + 1` won't be included as parameter. We
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
> Probably, we'd better to move this discussion to the jira?
I summary it and also post the design doc link to the jira.
---
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
> can we have a google doc so that we can leave comments inline? thanks!
Sure. The google doc is at
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/19813
Probably, we'd better to move this discussion to the jira?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
can we have a google doc so that we can leave comments inline? thanks!
---
-
To unsubscribe, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
I wrote a design doc at
https://paper.dropbox.com/doc/Split-deeply-nested-expressions-under-wholestage-codegen-WXkQ9iIlN3zkGdn8MHgiW
The design is based on what did in this PR.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
I'm not sure, it depends on how many places we already doing it and what's
the drawback if we forbid it. Let's have a design doc and gather more feedbacks.
Thanks for your understanding!
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
@cloud-fan Do you think we should still allow something like `a + 1` as the
output of codegen?
---
-
To unsubscribe, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
Also needs to revert #19969 which is based on this.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
@cloud-fan Ok. Thanks.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
After some more thoughts, this PR makes a new contract that Spark doesn't
promise before: `Expression.genCode` must output something that can be used as
parameter name or literal.
I do
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19813
This needs a design doc.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19813
thanks, merging to master!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84769/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84769 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84769/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84769 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84769/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84760/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84760 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84760/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84760 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84760/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84756 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84756/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84756/
Test FAILed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test FAILed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84756 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84756/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test FAILed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84742/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84742 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84742/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test FAILed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84741/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84741 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84741/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84742 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84742/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84741 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84741/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test FAILed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84710/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84710 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84710/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84710 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84710/testReport)**
for PR 19813 at commit
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
ping @cloud-fan Previous comments are all addressed. Please review this
again. Thanks.
---
-
To unsubscribe, e-mail:
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84641/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84641 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84641/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84642/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84642 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84642/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84642 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84642/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84641 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84641/testReport)**
for PR 19813 at commit
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/19813
LGTM, good job!
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
ping @kiszk @cloud-fan any more thoughts or review for this?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84486/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84486 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84486/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84486 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84486/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84422/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84422 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84422/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84422 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84422/testReport)**
for PR 19813 at commit
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
ping @cloud-fan @kiszk Do you have more review on this? Thanks.
---
-
To unsubscribe, e-mail:
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/19813
thanks @viirya , LGTM
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84371/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84371 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84371/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test FAILed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84370/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84370 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84370/testReport)**
for PR 19813 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84367/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84367 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84367/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84371 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84371/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84370 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84370/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84367 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84367/testReport)**
for PR 19813 at commit
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19813
@mgaido91 Thanks for the review.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands,
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84331/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19813
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84331 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84331/testReport)**
for PR 19813 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19813
**[Test build #84330 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84330/testReport)**
for PR 19813 at commit
1 - 100 of 115 matches
Mail list logo
