Skip to content

Add "full_outer" name to join types#17985

Closed
BartekH wants to merge 6 commits intoapache:masterfrom
BartekH:patch-1
Closed

Add "full_outer" name to join types#17985
BartekH wants to merge 6 commits intoapache:masterfrom
BartekH:patch-1

Conversation

@BartekH
Copy link
Copy Markdown
Contributor

@BartekH BartekH commented May 15, 2017

I have discovered that "full_outer" name option is working in Spark 2.0, but it is not printed in exception. Please verify.

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

I have discovered that "full_outer" name option is working in Spark 2.0, but it is not printed in exception. Please verify.
@wzhfy
Copy link
Copy Markdown
Contributor

wzhfy commented May 15, 2017

"full_outer" will be replaced by "fullouter" after .replace("_", ""), right?

def apply(typ: String): JoinType = typ.toLowerCase(Locale.ROOT).replace("_", "") match {
case "inner" => Inner
case "outer" | "full" | "fullouter" => FullOuter
case "outer" | "full" | "fullouter" | "full_outer" => FullOuter
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as @wzhfy points out, given that we remove underscores, this matching case is unnecessary. The same is true for left_outer, right_outer and left_semi.

@BartekH
Copy link
Copy Markdown
Contributor Author

BartekH commented May 17, 2017

That's ok, but "full_outer" is invisible in exception information. Please consider add it, because is is possible to use "full_outer". When wrong name of join option is provided and exception occurs, there is no information about "full_outer" option. It's a little bit misleading.

@wzhfy
Copy link
Copy Markdown
Contributor

wzhfy commented May 19, 2017

@BartekH Yes, we can add that to exception message. Please also add a test case for checking supported join types.

@HyukjinKwon
Copy link
Copy Markdown
Member

Ping @BartekH, how is it going?

@BartekH
Copy link
Copy Markdown
Contributor Author

BartekH commented Jun 6, 2017

pull request for JoinTypes tests has been created here: #18219

@BartekH BartekH mentioned this pull request Jun 6, 2017
add Apache license on the top of file
@HyukjinKwon
Copy link
Copy Markdown
Member

Could we just fold the changes in 18219 to here?

@BartekH
Copy link
Copy Markdown
Contributor Author

BartekH commented Jun 8, 2017

@HyukjinKwon Done.

class JoinTypesTest extends SparkFunSuite {

test("construct an Inner type") {
assert(JoinType.apply("inner") === Inner)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: can just have JoinType("...") here and below. Also, can we have a test that checks for the correct exception for wrong join types? Something along the lines of:

val ex = intercept[IllegalArgumentException] {
  JoinType.apply("wrong_join")
}
ex.getMessage() should contain "..."

@sameeragarwal
Copy link
Copy Markdown
Member

ok to test

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 9, 2017

Test build #77823 has finished for PR 17985 at commit aed0a06.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class RegisteredWorker(
  • public class JavaChiSquareTestExample
  • public class JavaCorrelationExample
  • class FilteredObjectInputStream extends ObjectInputStream
  • String.format(\"Unexpected class in stream: %s\", desc.getName()));
  • class Imputer @Since(\"2.2.0\") (@Since(\"2.2.0\") override val uid: String)
  • class DecisionTreeClassifierWrapperWriter(instance: DecisionTreeClassifierWrapper)
  • class DecisionTreeClassifierWrapperReader extends MLReader[DecisionTreeClassifierWrapper]
  • class DecisionTreeRegressorWrapperWriter(instance: DecisionTreeRegressorWrapper)
  • class DecisionTreeRegressorWrapperReader extends MLReader[DecisionTreeRegressorWrapper]
  • class HasMinSupport(Params):
  • class HasNumPartitions(Params):
  • class HasMinConfidence(Params):
  • case class UnresolvedRelation(
  • case class DayOfWeek(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
  • case class Cot(child: Expression)
  • case class Uuid() extends LeafExpression
  • case class InterpretedPredicate(expression: Expression) extends BasePredicate
  • case class StringReplace(srcExpr: Expression, searchExpr: Expression, replaceExpr: Expression)
  • case class Chr(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
  • trait Command extends LogicalPlan
  • case class UnresolvedHint(name: String, parameters: Seq[Any], child: LogicalPlan)
  • case class ResolvedHint(child: LogicalPlan, hints: HintInfo = HintInfo())
  • case class HintInfo(
  • public final class ParquetDictionary implements Dictionary
  • case class ExecutedCommandExec(cmd: RunnableCommand, children: Seq[SparkPlan]) extends SparkPlan
  • case class StateStoreId(
  • class UnsafeRowPair(var key: UnsafeRow = null, var value: UnsafeRow = null)
  • trait StateStoreWriter extends StatefulOperator

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 28, 2017

Test build #78787 has started for PR 17985 at commit 9fc9a0a.

@BartekH
Copy link
Copy Markdown
Contributor Author

BartekH commented Jun 28, 2017

Please rerun jenkins job.

@gatorsmile
Copy link
Copy Markdown
Member

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jul 3, 2017

Test build #79071 has finished for PR 17985 at commit 9fc9a0a.

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

@BartekH
Copy link
Copy Markdown
Contributor Author

BartekH commented Jul 3, 2017

It has failed again. It's not my fault, please retest it one more time.

@gatorsmile
Copy link
Copy Markdown
Member

retest this please

1 similar comment
@wzhfy
Copy link
Copy Markdown
Contributor

wzhfy commented Jul 4, 2017

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jul 4, 2017

Test build #79126 has finished for PR 17985 at commit 9fc9a0a.

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

@BartekH
Copy link
Copy Markdown
Contributor Author

BartekH commented Jul 4, 2017

Does your Jenkins work properly? It failed again, and again cause is different.

@gatorsmile
Copy link
Copy Markdown
Member

test this please

@gatorsmile
Copy link
Copy Markdown
Member

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 6, 2017

Test build #80296 has finished for PR 17985 at commit 9fc9a0a.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Copy Markdown
Member

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 6, 2017

Test build #80310 has finished for PR 17985 at commit 9fc9a0a.

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

@gatorsmile
Copy link
Copy Markdown
Member

LGTM

@gatorsmile
Copy link
Copy Markdown
Member

Thanks! Merging to master.

@asfgit asfgit closed this in 438c381 Aug 6, 2017
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.

6 participants