Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21537
@HyukjinKwon sorry for being late. I was swampped with several things. I
have just submitted it. Looking forward to seeing feedback.
---
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
@kiszk, have we created a JIRA or sent an email to dev mailing list yet? or
am I missing something already that happened?
---
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21537
Sure, let me create a JIRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
@kiszk Please create a JIRA and we can post more ideas there. Thanks!
---
-
To unsubscribe, e-mail:
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
I think we can set up a place (mailling list or JIRA) to discuss the
further thing about IR design, as suggested by @HyukjinKwon. This can be a
co-work from interesting parties.
---
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
@kiszk The initial prototype or proof of concept can be in any personal
branch. When we merge it to the master branch, we still need to separate it
from the current codegen and make it
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
If we will continue on improving current codegen framework, I think it is
good to have a design doc reviewed by the community.
If we decide to have IR design and get rid of this string based
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21537
It's a good point that we should have a design doc for this codegen
infrastructure improvement, since it's very critical to Spark. And we should
have it reviewed by the community.
There
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
I don't see a notable risk here. That's just to avoid string interpolation,
which makes less error-prone, which is discussed already and the code change is
small.
I hope we can move
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21537
@gatorsmile Thank you for your reply. Could you elaborate on your
suggestion?
>A general suggestion. To avoid introducing the regressions, how about
implementing a new one without changing the
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
@HyukjinKwon I am worrying about the design of a mixture of representation
s"" and code""? When the design is not good, it is hard to maintain it and add
new code based on this. Let @cloud-fan
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
Thanks for all involving in this discussion!
Sorry that I was on a long flight and feels too tired to reply now. I just
want to say for now, no matter what decision we made, to continue to
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21537
Thanks everyone for this discussion. I think we all agree that what we need
as a first thing is a design doc and we can move the discussion there.
@kiszk thank you for your comments. I
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
Sounds cool, let's move both to JIRAs or mailing lists.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21537
For 2. and 3., it is harder to say my opinion in the comment. Let me say
short comments at first.
For 2., if I remember correctly, @viirya once wrote the API document in a
JIRA entry. it
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
If there's a bug, then let's fix in another JIRA. If that's impossible to
fix or sounds super risky and there's something I missed, let's revert.
---
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
I wouldn't revert this unless there are specific concerns about this. Do
you see any bug by a mixture
of representation `s""` and `code""`?
---
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21537
Thank for involving me in an important thread. I was busy this morning in
Japan.
I think there are three topics in the thread.
1. Merge or revert this PR
2. Design document
3. IR
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
We are fully swamped by the hotfix and regressions of 2.3 release and the
new features that are targeting to 2.4. We should post some comments in this PR
earlier.
Designing an IR for
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
Yea, yea. I understand. I wasn't trying to say the severity of effect by
introducing AnalysisBarrier wasn't trivial. True, I understand it is not an
easy job. Thank you Reynold for that. Yea,
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
To Spark users, introducing AnalysisBarrier is a disaster. However, to the
developers of Spark internal, this is just a bug. If you served the customers
who are heavily using Spark, you will
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
I know and agree it ended up with many regressions and bugs (strictly I
meant those by behaviour changes, which was my mistake). I have been following
that and I am glad that it has been
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
I am fine to not revert it since it is too late. So many related PRs have
been merged, but we need to seriously consider writing and reviewing the design
docs before changing the code generation
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
AnalysisBarrier does not introduce a behavior change. However, this
requires our analyzer rules must be idempotent. The most recent correctness bug
also shows another big potential hole
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
I am sorry if I misunderstood. This was discussed sufficiently in the
linked PRs. These are internal APIs and does not introduce behaviour changes by
AnalysisBarrier which was a good enough
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
This is not related to the credit or not. I think we need to introduce an
IR like what the compiler is doing instead of continuous improvement on the
existing one, which is already very hacky
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21537
Sure, I am fine with @kiszk leading the effort of course, I just wanted to
give credit to @viirya for the (current) design as it was mostly done by him.
---
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
This is bad. The design needs to be carefully reviewed before implementing
it. Basically, we breaks the basic principles of software engineering. It is
very strange to write the design doc after
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21537
@gatorsmile I think this is just the partial adoption of what was proposed
[here](https://github.com/apache/spark/pull/19813#issuecomment-354045400) and
implemented in SPARK-24121
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
@kiszk You are a JVM expert with a very strong IR background. Could you
lead the efforts and drive the IR design?
---
-
To
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
Another example is the AnalysisBarrier, which becomes a disaster to Spark
2.3 release. Many blockers, correctness bugs, performance regressions are
caused by that. Thus, I think we should revert
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/21537
Based on this PR, so many changes will be made in the codegen. The codegen
is very fundamental to Spark SQL. I do not think we should merge this PR at
this stage. To be more disciplined, we need
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
Merged 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/21537
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/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94782/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #94782 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94782/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
**[Test build #94782 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94782/testReport)**
for PR 21537 at commit
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
retest this please
---
-
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/21537
Thanks @HyukjinKwon
---
-
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/21537
@HyukjinKwon Thanks! I've updated. Please let me know if they are good now.
---
-
To unsubscribe, e-mail:
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/21537
@viirya, mind if I ask to update the PR description? Looks proposed APIs
were slightly changed.
---
-
To unsubscribe,
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94654/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #94654 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94654/testReport)**
for PR 21537 at commit
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21537
LGTM
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #94654 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94654/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
Thanks @mgaido91
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail:
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/21537
LGTM apart from this
[comment](https://github.com/apache/spark/pull/21537#discussion_r209353035)
---
-
To unsubscribe, e-mail:
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93200/
Test PASSed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
**[Test build #93200 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93200/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93200 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93200/testReport)**
for PR 21537 at commit
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
retest this please.
---
-
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/21537
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93179/
Test FAILed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
**[Test build #93179 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93179/testReport)**
for PR 21537 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93179 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93179/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
retest this please.
---
-
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/21537
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93151/
Test FAILed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
**[Test build #93151 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93151/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93151 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93151/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21537
retest this please
---
-
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/21537
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/21537
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93141/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93141 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93141/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
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/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93141 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93141/testReport)**
for PR 21537 at commit
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
retest this please.
---
-
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/21537
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/21537
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93132/
Test FAILed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93132 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93132/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
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/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93117/
Test PASSed.
---
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93117 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93117/testReport)**
for PR 21537 at commit
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93132 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93132/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21537
**[Test build #93117 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93117/testReport)**
for PR 21537 at commit
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21537
retest this please.
---
-
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/21537
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93108/
Test FAILed.
---
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
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/21537
**[Test build #93108 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93108/testReport)**
for PR 21537 at commit
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21537
Merged build finished. Test PASSed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional
1 - 100 of 227 matches
Mail list logo
