Skip to content

Conversation

@petermaxlee
Copy link
Contributor

@petermaxlee petermaxlee commented Aug 18, 2016

What changes were proposed in this pull request?

This patch refactors type widening and makes the usage of them in expressions more consistent.

Before this patch, we have the following 6 functions for type widening. The names are inconsistent.

findTightestCommonTypeOfTwo (binary version)
findTightestCommonTypeToString (binary version)
findTightestCommonTypeAndPromoteToString (n-ary version)
findTightestCommonType (n-ary version)
findWiderTypeForTwo (binary version)
findWiderCommonType (n-ary version)

After this patch, we have the following 4 left:

widerType (binary version)
widerType (n-ary version)
widerTypeToString (binary version)
widerTypeToString (n-ary version)

This is another attempt at #14389

How was this patch tested?

WIP

@SparkQA
Copy link

SparkQA commented Aug 18, 2016

Test build #63973 has finished for PR 14696 at commit 9df4551.

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

@petermaxlee
Copy link
Contributor Author

@cloud-fan this actually broke decimal precision.

I'm starting to think that it would be better to push type coercion into each expression, and then the arithmetic can create special cases for decimal types before calling the functions provided here. It would be a much larger change though.

@cloud-fan
Copy link
Contributor

from the previous discussion(#14389 (comment)):

We also need to add some checks before applying the type widen rules, to avoid conflicting with DecimalPrecision, which defines some special rules for binary arithmetic about decimal type.

Have you tried this?

push type coercion into each expression is also in my plan, but it would be a very large change, we should have a design doc first and discuss it with some people. So I'd like to do this small refactor first, what do you think?

@petermaxlee
Copy link
Contributor Author

Closing this for now until I revisit.

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