Skip to content

Conversation

@cloud-fan
Copy link
Contributor

This PR fixes a bug introduced in #6505.
Decimal literal's value is not java.math.BigDecimal, but Spark SQL internal type: Decimal.

@cloud-fan
Copy link
Contributor Author

cc @marmbrus @rxin

@rxin
Copy link
Contributor

rxin commented Jun 2, 2015

LGTM

1 similar comment
@marmbrus
Copy link
Contributor

marmbrus commented Jun 2, 2015

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

btw we should rename this BooleanEquality instead of "Equalization".

@cloud-fan
Copy link
Contributor Author

Just realized decimal literal's type is not always DecimalType.Unlimited. User can do something like Literal.create(Decimal(1), DecimalType(8, 0)). Should we handle this case too? If so, I think we still need to compare the wrapped value for literals.

@rxin
Copy link
Contributor

rxin commented Jun 2, 2015

We should handle those as well ...

@SparkQA
Copy link

SparkQA commented Jun 2, 2015

Test build #33960 has finished for PR 6574 at commit 4d35e58.

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

@cloud-fan cloud-fan changed the title [SPARK-7952][SQL] compare Literals instead of its value in BooleanEqualization [SPARK-7952][SQL] use internal Decimal instead of java.math.BigDecimal Jun 2, 2015
@SparkQA
Copy link

SparkQA commented Jun 2, 2015

Test build #33963 has finished for PR 6574 at commit 90eeb25.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only care about boolean type and literal type here, not left and right, so I use bool instead.

@SparkQA
Copy link

SparkQA commented Jun 7, 2015

Test build #34392 has finished for PR 6574 at commit b0e3549.

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

@rxin
Copy link
Contributor

rxin commented Jun 7, 2015

Thanks. Merging this in.

cloud-fan added a commit that referenced this pull request Jun 7, 2015
This PR fixes a bug introduced in #6505.
Decimal literal's value is not `java.math.BigDecimal`, but Spark SQL internal type: `Decimal`.

Author: Wenchen Fan <[email protected]>

Closes #6574 from cloud-fan/fix and squashes the following commits:

b0e3549 [Wenchen Fan] rename to BooleanEquality
1987b37 [Wenchen Fan] use Decimal instead of java.math.BigDecimal
f93c420 [Wenchen Fan] compare literal
@rxin
Copy link
Contributor

rxin commented Jun 7, 2015

btw I see the following compilation warning for master branch just now. Can you take a look?

[warn] /scratch/rxin/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala:448: fruitless type test: a value of type org.apache.spark.sql.catalyst.expressions.Expression cannot also be a org.apache.spark.sql.types.DecimalType
[warn]         case b @ BinaryComparison(e1 @ DecimalType.Fixed(_, _), e2)
[warn]                                                         ^
[warn] /scratch/rxin/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala:451: fruitless type test: a value of type org.apache.spark.sql.catalyst.expressions.Expression cannot also be a org.apache.spark.sql.types.DecimalType
[warn]         case b @ BinaryComparison(e1, e2 @ DecimalType.Fixed(_, _))

@cloud-fan
Copy link
Contributor Author

It's a mistake made in #6405, we should use DecimalType.Expression instead of DecimalType.Fixed to match expression. Sorry about it...

@SparkQA
Copy link

SparkQA commented Jun 8, 2015

Test build #34406 has finished for PR 6574 at commit d7b60c8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class HasCachedBlocks(executorId: String) extends ToBlockManagerMaster
    • class KernelDensity(object):

@rxin
Copy link
Contributor

rxin commented Jun 8, 2015

Can you submit a separate pull request? This patch has actually been merged already, but ASF's github sync is behind so it is not showing up here.

@rxin
Copy link
Contributor

rxin commented Jun 8, 2015

Please close this pull request also (since it's already been merged).

You can see the up-to-date merges here: https://git-wip-us.apache.org/repos/asf?p=spark.git

@cloud-fan cloud-fan closed this Jun 8, 2015
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
This PR fixes a bug introduced in apache#6505.
Decimal literal's value is not `java.math.BigDecimal`, but Spark SQL internal type: `Decimal`.

Author: Wenchen Fan <[email protected]>

Closes apache#6574 from cloud-fan/fix and squashes the following commits:

b0e3549 [Wenchen Fan] rename to BooleanEquality
1987b37 [Wenchen Fan] use Decimal instead of java.math.BigDecimal
f93c420 [Wenchen Fan] compare literal
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.

4 participants