Skip to content

SPARK-3329: [SQL] Don't depend on Hive SET pair ordering.#2220

Closed
willb wants to merge 3 commits intoapache:masterfrom
willb:spark-3329
Closed

SPARK-3329: [SQL] Don't depend on Hive SET pair ordering.#2220
willb wants to merge 3 commits intoapache:masterfrom
willb:spark-3329

Conversation

@willb
Copy link
Contributor

@willb willb commented Aug 31, 2014

This fixes some possible spurious test failures in HiveQuerySuite by comparing sets of key-value pairs as sets, rather than as lists.

@concretevitamin
Copy link
Contributor

concretevitamin commented Aug 31, 2014

ok to test

@SparkQA
Copy link

SparkQA commented Aug 31, 2014

QA tests have started for PR 2220 at commit 36ff52a.

  • This patch merges cleanly.

@concretevitamin
Copy link
Contributor

concretevitamin commented Aug 31, 2014

Ah, so this problem was fixed in PR #1514, but it seems like this later PR reverted the change accidentally. I think it'd be good to re-adapt 1514's solution.

/cc @aarondav @liancheng

@willb
Copy link
Contributor Author

willb commented Aug 31, 2014

Thanks, @concretevitamin!

@SparkQA
Copy link

SparkQA commented Aug 31, 2014

QA tests have finished for PR 2220 at commit 36ff52a.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

aarondav and others added 2 commits September 2, 2014 08:48
@aarondav @willb
Fix flakey HiveQuerySuite test
cf11b0e 
Result may not be returned in the expected order, so relax that constraint.

Author: Aaron Davidson <aaron@databricks.com>

Closes apache#1514 from aarondav/flakey and squashes the following commits:

e5af823 [Aaron Davidson] Fix flakey HiveQuerySuite test

Conflicts:
	sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala
@willb
Handle cases where SET returns Rows of (single) strings
6525d8e
@willb
Copy link
Contributor Author

willb commented Sep 2, 2014

@concretevitamin I cherry-picked @aarondav's fix (and added a very simple fix to handle cases that it didn't).

@SparkQA
Copy link

SparkQA commented Sep 2, 2014

QA tests have started for PR 2220 at commit 6525d8e.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 2, 2014

QA tests have finished for PR 2220 at commit 6525d8e.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

marmbrus
marmbrus reviewed Sep 3, 2014
View reviewed changes
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala Outdated
Copy link
Contributor

@marmbrus marmbrus Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style nit: this could just be: case Row(KV(key,value)) => ... I believe.

@marmbrus
Copy link
Contributor

marmbrus commented Sep 3, 2014

Hey @willb, thanks for looking into / fixing this! Minor pattern matching suggestion only.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have started for PR 2220 at commit 3b3e205.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 3, 2014

QA tests have finished for PR 2220 at commit 3b3e205.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@willb
Copy link
Contributor Author

willb commented Sep 3, 2014

This failure (in SparkSubmitSuite) appears unrelated to my patch.

@SparkQA
Copy link

SparkQA commented Sep 5, 2014

QA tests have started for PR 2220 at commit 3b3e205.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 6, 2014

Tests timed out after a configured wait of 120m.

@marmbrus
Copy link
Contributor

marmbrus commented Sep 9, 2014

Thanks for cleaning this up! Since this passed tests before I'm going to merge to master.

@asfgit asfgit closed this in 2b7ab81 Sep 9, 2014
@JoshRosen
Copy link
Contributor

JoshRosen commented Feb 8, 2015

@marmbrus @liancheng Can we backport this into branch-1.1 (1.1.2)? I've been observing a lot of flakiness in the "HiveQuerySuite.SET commands semantics for a HieContext" suite in branch-1.1 and it sounds like this patch might fix it. There are a couple of merge conflicts, but I think they should be simple to fix.

@JoshRosen JoshRosen mentioned this pull request Feb 9, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@willb @concretevitamin @SparkQA @marmbrus @JoshRosen @aarondav