[CALCITE-5651] Inferred scale for decimal should not exceed maximum allowed scale#3156
Conversation
|
@mbudiu-vmw I think the PR makes sense. There are some style errors though, could you please take care of them? |
| int maxScale = factory.getTypeSystem().getMaxNumericScale(); | ||
| // scale should not greater than precision. | ||
| int scale = maxPrecision / 2; | ||
| int scale = Math.min(maxPrecision / 2, maxScale); |
There was a problem hiding this comment.
Personally I think that the choice of maxPrecision/2 is rather arbitrary, but I didn't want to change too many things at once. This used to be maxScale, and that should always be smaller than maxPrecision.
mihaibudiu
force-pushed
the
issue5651
branch
2 times, most recently
from
3650f79 to
6ec977a
Compare
|
@mbudiu-vmw Small tip: it's better to avoid force-pushes after a review has started cause it makes review history hard to follow; no big deal for this change but it can be a pain in larger changes. |
rubenada
added
LGTM-will-merge-soon
|
@zabetak this PR needs CALCITE-5650; if I understand correctly, you plan to take care of that, so I'll pause the merge of the current PR until then, ok? |
|
@rubenada Sounds good let's do that! |
|
CALCITE-5650; has been merged (thanks @zabetak); @mbudiu-vmw could you please revisit this PR? |
|
I have rebased. How does one run the style checkers locally before submission? |
If I am not mistaken a local |
|
LGTM. |
rubenada
added
the
LGTM-will-merge-soon
|
Changed the commit message. |
|
Kudos, SonarCloud Quality Gate passed! |
3 participants
This PR includes the fix in #3155 as well, because it depends on it.