Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-1997] update breeze to version 0.8.1 #940

Closed
wants to merge 1 commit into from

Conversation

witgo
Copy link
Contributor

@witgo witgo commented Jun 2, 2014

breeze 0.8.1 dependent on scala-logging-slf4j 2.1.1 The relevant code on #1369

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@witgo witgo changed the title update breeze to version 0.8.1 [WIP] update breeze to version 0.8.1 Jun 2, 2014
@AmplabJenkins
Copy link

Merged build finished. All automated tests passed.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15348/

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@markhamstra
Copy link
Contributor

What is the reason for this change, and how does it affect our intention to maintain binary compatibility?

@AmplabJenkins
Copy link

Merged build finished. All automated tests passed.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15350/

@rxin
Copy link
Contributor

rxin commented Jun 2, 2014

I don't think we ever expose Breeze in public facing API, do we?

@markhamstra
Copy link
Contributor

We shouldn't, so I think we maintain source compatibility without any trouble. Are the MIMA checks good enough to catch binary incompatibility when we make significant changes to library dependencies? Not that I am asserting that this Breeze change is particularly significant, since I haven't look at the diff from 0.7. I'm more interested in the general case of what needs to be checked or done when we want to update dependencies while staying within the bounds of 1.x.

@witgo witgo changed the title [WIP] update breeze to version 0.8.1 [SPARK-1997] update breeze to version 0.8.1 Jun 3, 2014
@witgo
Copy link
Contributor Author

witgo commented Jun 3, 2014

@markhamstra , breeze 0.7 does not support scala 2.11 .

@markhamstra
Copy link
Contributor

Neither does spark 1.0.0.

We've offered no guarantee that any spark 1.x will work with scala 2.11. If it turns out that we can't cross-compile for scala 2.10 and 2.11 with essentially the same source code and dependencies and preserving our intention to preserve binary compatibility, then that may be the occasion that forces the beginning of spark 2.0 development.

In any event, the breaking of our intent to preserve binary compatibility is no small matter and shouldn't slip into a PR without considerable discussion. I'm not certain that that is what is happening with this PR, but I do think that it is critical that we understand how strong our checks for binary compatibility are and what are the bounds on dependency upgrades that we can make while preserving that compatibility.

@witgo witgo changed the title [SPARK-1997] update breeze to version 0.8.1 [WIP][SPARK-1997] update breeze to version 0.8.1 Jun 3, 2014
@witgo
Copy link
Contributor Author

witgo commented Jun 3, 2014

Speaking on spark, breeze no big change from 0.7 to 0.8.1. Of course, this conclusion has not been a lot of testing

@mengxr
Copy link
Contributor

mengxr commented Jun 4, 2014

Let's put this PR on hold until Spark moves to Scala 2.11. Good to see that the breeze functions we used did not change in breeze-0.8.1.

@ktham
Copy link

ktham commented Jun 4, 2014

Just looked at the release notes for Breeze 0.8 and initial glance at the commits... is there any particular reason why we are holding back this PR? If we want to cross compile with Scala 2.11, then we have to upgrade to breeze 0.8 because there does not exist a 2.11 build of breeze 0.7

We'll need to upgrade eventually.

@mengxr
Copy link
Contributor

mengxr commented Jun 5, 2014

@ktham I think @markhamstra 's comments addressed the concerns very well. Unless there are critical bugs in breeze 0.7, we should wait until Spark core starts to move. Breeze has a fast release cycle. So we don't need to rush to 0.8.1.

@witgo witgo changed the title [WIP][SPARK-1997] update breeze to version 0.8.1 [SPARK-1997] update breeze to version 0.8.1 Jun 6, 2014
@nevillelyh
Copy link
Contributor

We just hit a bug when DenseVector in 0.7 triggers StackOverflowError in KryoSerializer.

@mengxr
Copy link
Contributor

mengxr commented Jun 19, 2014

@nevillelyh Is there a JIRA for it? Is it fixed in 0.8.1?

@nevillelyh
Copy link
Contributor

@SparkQA
Copy link

SparkQA commented Jul 11, 2014

QA tests have started for PR 940. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16545/consoleFull

@SparkQA
Copy link

SparkQA commented Jul 11, 2014

QA results for PR 940:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16545/consoleFull

@SparkQA
Copy link

SparkQA commented Jul 19, 2014

QA tests have started for PR 940. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16848/consoleFull

@mengxr
Copy link
Contributor

mengxr commented Jul 21, 2014

Jenkins, retest this please.

1 similar comment
@mengxr
Copy link
Contributor

mengxr commented Jul 28, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jul 28, 2014

QA tests have started for PR 940. This patch DID NOT merge cleanly!
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/17265/consoleFull

@avati
Copy link
Contributor

avati commented Aug 1, 2014

@mengxr Based on the dependency graph, I am guessing we will now have jar hell problem with scalalogging-slf4j 2.1.2 (needed by breeze 0.8.1) vs 1.0.1 (used by sql/). Breeze 0.7 was depending on 0.7 too. But 2.1.2 and 1.0.1 are not compatible.

So #1701 (or equivalent) needs to be merged soon to resolve the issue.

@srowen
Copy link
Member

srowen commented Aug 1, 2014

Is that a problem? just update sql/catalyst to use 2.1.2? Do we know if that would be any problem? @witgo has a PR to standardize use of scalalogging-slf4j anyway (#1369) which bumps to 2.1.2 and seems to have worked. Maybe that should go in with this to ensure all is compatible.

@mengxr
Copy link
Contributor

mengxr commented Aug 1, 2014

Ah, I see. Tests were against individual build instead of the assembly jar. We should have integration tests in the future.

@avati
Copy link
Contributor

avati commented Aug 1, 2014

Yes, either #1701 or #1369. We are already broken till they are committed.

On Fri, Aug 1, 2014 at 8:19 AM, Xiangrui Meng notifications@github.com
wrote:

Ah, I see. Tests were against individual build instead of the assembly
jar. We should have integration tests in the future.


Reply to this email directly or view it on GitHub
#940 (comment).

@asfgit asfgit closed this in 0dacb1a Aug 1, 2014
@mengxr
Copy link
Contributor

mengxr commented Aug 1, 2014

I think Jenkins should be fine but the assembly jar is broken. Is it right?

@avati
Copy link
Contributor

avati commented Aug 1, 2014

I think Jenkins should be fine but the assembly jar is broken. Is it right?

I think so, just like commons-math3

@witgo
Copy link
Contributor Author

witgo commented Aug 1, 2014

How do we resolve this issue?

@mengxr
Copy link
Contributor

mengxr commented Aug 1, 2014

I reverted the change in #1718 and asked @marmbrus to take look at the dependency issues caused by scala-logging.

@srowen
Copy link
Member

srowen commented Aug 1, 2014

I think @witgo's PR strongly suggests it's OK to upgrade to 2.1.2?

asfgit pushed a commit that referenced this pull request Aug 1, 2014
breeze-0.8.1 causes dependency issues, as discussed in #940 .

Author: Xiangrui Meng <meng@databricks.com>

Closes #1718 from mengxr/revert-breeze and squashes the following commits:

99c4681 [Xiangrui Meng] downgrade breeze version to 0.7
@witgo witgo deleted the breeze-8.0.1 branch August 1, 2014 17:17
@avati
Copy link
Contributor

avati commented Aug 1, 2014

I reverted the change in #1718 #1718
and asked @marmbrus https://github.com/marmbrus to take look at the
dependency issues caused by scala-logging.

The other option could have been to keep #940 merged and merge also #1701
(if #1369 is too invasive/requires more review), that combination seems to
work at least for me.

@marmbrus
Copy link
Contributor

marmbrus commented Aug 1, 2014

I'm okay with upgrading the version of scala logging that we use as @avati suggests.

xiliu82 pushed a commit to xiliu82/spark that referenced this pull request Sep 4, 2014
`breeze 0.8.1`  dependent on  `scala-logging-slf4j 2.1.1` The relevant code on apache#1369

Author: witgo <witgo@qq.com>

Closes apache#940 from witgo/breeze-8.0.1 and squashes the following commits:

65cc65e [witgo] update breeze  to version 0.8.1
xiliu82 pushed a commit to xiliu82/spark that referenced this pull request Sep 4, 2014
breeze-0.8.1 causes dependency issues, as discussed in apache#940 .

Author: Xiangrui Meng <meng@databricks.com>

Closes apache#1718 from mengxr/revert-breeze and squashes the following commits:

99c4681 [Xiangrui Meng] downgrade breeze version to 0.7
Agirish pushed a commit to HPEEzmeral/apache-spark that referenced this pull request May 5, 2022
wangyum added a commit that referenced this pull request May 26, 2023
…& Sum which handle by themself internally (#940)

* Fix Number of partitions (0) must be positive

* [SPARK-34037][SQL] Remove unnecessary upcasting for Avg & Sum which handle by themself internally

### What changes were proposed in this pull request?
The type-coercion for numeric types of average and sum is not necessary at all, as the resultType and sumType can prevent the overflow.

### Why are the changes needed?

rm unnecessary logic which may cause potential performance regressions

### Does this PR introduce _any_ user-facing change?

no
### How was this patch tested?

tpcds tests for plan

Closes #31079 from yaooqinn/SPARK-34037.

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

(cherry picked from commit a235c3b)

* [SPARK-34037][SQL] Remove unnecessary upcasting for Avg & Sum which handle by themself internally

### What changes were proposed in this pull request?
The type-coercion for numeric types of average and sum is not necessary at all, as the resultType and sumType can prevent the overflow.

### Why are the changes needed?

rm unnecessary logic which may cause potential performance regressions

### Does this PR introduce _any_ user-facing change?

no
### How was this patch tested?

tpcds tests for plan

Closes #31079 from yaooqinn/SPARK-34037.

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

(cherry picked from commit a235c3b)

* fix

Co-authored-by: Kent Yao <yao@apache.org>
udaynpusa pushed a commit to mapr/spark that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.