Skip to content

Conversation

@cloud-fan
Copy link
Contributor

based on #7348

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37365 has finished for PR 7420 at commit 5cca5ee.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class BinaryOperator extends BinaryExpression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why we need isSameType? I think the logic should be:

  1. first check whether the input type is acceptable(i.e. expectedType.acceptsType(inType) returns true). If it is, do nothing here.
  2. If input type is not acceptable, follow cast rules below to do implicit type cast.

It looks to me we only need acceptsType.

Copy link
Contributor

Choose a reason for hiding this comment

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

there was a reason, but i can't remember why right now. let me think about it.

cc @marmbrus do you remember why?

@cloud-fan
Copy link
Contributor Author

cc @rxin

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37368 has finished for PR 7420 at commit 03b70da.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class BinaryOperator extends BinaryExpression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better to generate a different error message for BinaryOperator, as 2 children have same type and same expected type, we don't need to follow ExpectsInputTypes to report type error for both left and right.

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37376 has finished for PR 7420 at commit fe169b0.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class BinaryOperator extends BinaryExpression

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37384 has finished for PR 7420 at commit 5775081.

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

@SparkQA
Copy link

SparkQA commented Jul 15, 2015

Test build #37392 has finished for PR 7420 at commit 7633fa9.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? Imagine we have TypeCollection(LongType, StringType), and the input is StringType

Wouldn't this just cast input to a longtype?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the first rule is: case _ if expectedType.acceptsType(inType) => e.
So when we reach here, input type is not acceptable for any types in this type collection. see the inline comments above.

@rxin
Copy link
Contributor

rxin commented Jul 16, 2015

LGTM - @marmbrus and I can't really think of why anymore we needed it - maybe we don't need it anymore. Going to merge this.

@asfgit asfgit closed this in ba33096 Jul 16, 2015
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.

3 participants