Skip to content

[SPARK-8371][SQL] improve unit test for MaxOf and MinOf and fix bugs #6825

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SPARK-8371][SQL] improve unit test for MaxOf and MinOf and fix bugs #6825

wants to merge 1 commit into from

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Jun 15, 2015

a follow up of #6813

cloud-fan
cloud-fan reviewed Jun 15, 2015
View reviewed changes
...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Jun 15, 2015

Choose a reason for hiding this comment

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

This test will fail at checkEvaluationWithGeneratedProjection for hash code not equal. I'm not sure why we want to compare the hash code, and I looked into the hashCode implementation of GenericRow and Row, they are different. I'm trying to figure it out.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Jun 15, 2015

cc @rxin @davies

cloud-fan
cloud-fan reviewed Jun 15, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Jun 15, 2015

Choose a reason for hiding this comment

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

The origin code is not right as super here refers to BinaryArithmetic, not Expression that we wanted.

Copy link
Contributor

@lianhuiwang lianhuiwang Jun 15, 2015

Choose a reason for hiding this comment

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

yes,i think you are right. because their symbol is empty.

@SparkQA
Copy link

SparkQA commented Jun 15, 2015

Test build #34923 has finished for PR 6825 at commit cd51a7d.

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

@SparkQA
Copy link

SparkQA commented Jun 15, 2015

Test build #34933 has finished for PR 6825 at commit 03021bc.

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

lianhuiwang
lianhuiwang reviewed Jun 15, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala Outdated
Copy link
Contributor

@lianhuiwang lianhuiwang Jun 15, 2015

Choose a reason for hiding this comment

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

i think at first it needs to check whether eval1 or eval2 isNull. and then calculate primitive.

Copy link
Contributor Author

@cloud-fan cloud-fan Jun 15, 2015

Choose a reason for hiding this comment

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

the code result is inserted after null check.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Jun 15, 2015

retest it please.

@davies
Copy link
Contributor

davies commented Jun 16, 2015

@cloud-fan There are some changes related to Max/Min in #6726 , is it useful for you? It will be good if you could merge them here.

@SparkQA
Copy link

SparkQA commented Jun 16, 2015

Test build #34987 has finished for PR 6825 at commit f153380.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class IsNull(child: Expression) extends UnaryExpression with Predicate
    • case class IsNotNull(child: Expression) extends UnaryExpression with Predicate

davies
davies reviewed Jun 17, 2015
View reviewed changes
...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala Outdated
Copy link
Contributor

@davies davies Jun 17, 2015

Choose a reason for hiding this comment

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

All expressions only work with InternalRow, which only contain internal types (int, long, UTF8String), no Date or Timestamp. so we don't need to test Date/Timestamp, or use DateUtils.fromDate() to do the conversion before calling expression on them.

cloud-fan
cloud-fan reviewed Jun 17, 2015
View reviewed changes
...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Jun 17, 2015

Choose a reason for hiding this comment

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

This test will fail at checkEvaluationWithGeneratedProjection for hash code not equal. I'm not sure why we want to compare the hash code, and I looked into the hashCode implementation of GenericRow and Row, they are different. Does anyone know why we want to compare the hash code there?

@SparkQA
Copy link

SparkQA commented Jun 17, 2015

Test build #35044 has finished for PR 6825 at commit 97066a6.

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

cloud-fan
cloud-fan reviewed Jun 17, 2015
View reviewed changes
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala Outdated
Copy link
Contributor Author

@cloud-fan cloud-fan Jun 17, 2015

Choose a reason for hiding this comment

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

@davies , we need the conversion here, as we use String instead of UTF8String at many tests...

Copy link
Contributor

@davies davies Jun 17, 2015

Choose a reason for hiding this comment

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

Sound reasonable, it's anonying to have so many UTF8String.fromString in test cases.

@SparkQA
Copy link

SparkQA commented Jun 17, 2015

Test build #35045 has finished for PR 6825 at commit 8c3fbde.

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

@cloud-fan cloud-fan mentioned this pull request Jun 18, 2015
Closed
@davies
Copy link
Contributor

davies commented Jun 23, 2015

@cloud-fan Now #6876 is merged, could you update this PR?

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35636 has finished for PR 6825 at commit 43170cc.

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

davies
davies reviewed Jun 24, 2015
View reviewed changes
...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
Copy link
Contributor

@davies davies Jun 24, 2015

Choose a reason for hiding this comment

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

Good catch!

@davies
Copy link
Contributor

davies commented Jun 24, 2015

LGTM, the failed PySpark test is not related, merging this into master, thanks!

@asfgit asfgit closed this in 09fcf96 Jun 24, 2015
@cloud-fan cloud-fan deleted the cg branch June 24, 2015 06:32
@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #955 has finished for PR 6825 at commit 43170cc.

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

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.

5 participants

@cloud-fan @SparkQA @davies @marmbrus @lianhuiwang