drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From daveoshinsky <...@git.apache.org>
Subject [GitHub] drill issue #517: DRILL-4704 fix
Date Mon, 08 Aug 2016 19:37:23 GMT
Github user daveoshinsky commented on the issue:

    https://github.com/apache/drill/pull/517
  
    The overflow problem will require separate changes to fix, from any changes we make in
the short-term to fix this issue (see DRILL-4834).  However, if I understand some of Paul's
findings earlier, the two issues are related as follows.  If we choose a large precision
based only on the kind of integer that is input to the cast (either an int, or a long, but
not minimized to represent the integer value to be casted), then we are more likely to encounter
the overflow issue, because this precision is more likely to exceed the capacity of the destination
decimal of the cast.  Is that right, Paul?  Given the risk of overflow, it is safer to choose
the absolute minimum precision possible, which is what the changes I made to CastIntDecimal.java
are doing.  We can't avoid overflow in every situation, but we can try to minimize the chances
of it.  My understanding is that the actual integer value to be casted isn't available in
ExpressionTreeMaterializer.java; at least, I did not see
  how to get it.  And that integer value is required to compute the minimum precision to
represent that value.  I don't see how to fix this problem reliably in ExpressionTreeMaterializer.java,
as I said earlier.  If you (Jinfeng) know how to do that, why don't you just take the changes
in PR-517, and then check changes in on top of those to fix the problem your way?  At least,
you get my unit test, which was for a condition (DRILL-4704) that was never tested by any
earlier unit test. 
    
        On Monday, August 8, 2016 3:25 PM, Jinfeng Ni <notifications@github.com> wrote:
     
    
     I'm not fully convinced that we should check the precision for each input value for this
castIntDecimal function. The argument of proposed patch is parameter "precision=0" is not
valid, and has to be calculated on-the-fly or "dynamically" for each input value. To me, 1)
if "precision=0" is a wrong input to this function, then we should fix the place where precision=0
is passed in. 2) why would you treat "precision=0" only? what if I pass in "precision = 1",
and a integer value of 123456? in such case, do we think "precison=1" is valid or not? Regarding
the overflow problem, that seems to be a separate issue, and probably is true for most of
the existing decimal functions. —
    You are receiving this because you authored the thread.
    Reply to this email directly, view it on GitHub, or mute the thread.  
    
      


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message