drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From daveoshinsky <...@git.apache.org>
Subject [GitHub] drill pull request #517: DRILL-4704 fix
Date Tue, 12 Jul 2016 16:01:04 GMT
Github user daveoshinsky commented on a diff in the pull request:

    https://github.com/apache/drill/pull/517#discussion_r70468695
  
    --- Diff: exec/java-exec/src/main/codegen/templates/Decimal/CastIntDecimal.java ---
    @@ -64,7 +64,15 @@ public void setup() {
     
         public void eval() {
             out.scale = (int) scale.value;
    -        out.precision = (int) precision.value;
    --- End diff --
    
    The DRILL-4704 bug root cause was that there is no supplied precision in this case (casting
an int to a decimal), i.e., the "supplied precision" was a bogus value of 0, and the casted
decimal with precision 0 matched nothing (unless perhaps the value was 0, but I didn't try
that).  The most direct way I could fix this was to compute the precision, which just involves
a few integer divides by 10 basically.  It might be possible to assume a large enough precision
(say 38, to fit most any integer), but that would waste lots of space (to hold a much smaller
value than worst case precision of 38 can hold).  Moreover, whenever one assumes a "worst
case", eventually it's likely to hit a "worse than worst case" (remember fixed length char
buffers in C?  128, no 256, no 1024, and so on).  It's been a while since I ran this (and
it was difficult to debug because of the runtime code generation and compilation), but it
may be the case that the cast from int to decimal is done only once 
 (to prepare a decimal for comparison, just once), in which case the divisions by 10 really
wouldn't make any difference. 
    
        On Tuesday, July 12, 2016 11:13 AM, Aman Sinha <notifications@github.com> wrote:
     
    
     In exec/java-exec/src/main/codegen/templates/Decimal/CastIntDecimal.java:> @@ -64,7
+64,15 @@ public void setup() {
    >  
    >      public void eval() {
    >          out.scale = (int) scale.value;
    > -        out.precision = (int) precision.value;
    I am not too familiar with the decimal type but taking a closer look at the fix I don't
think we should ignore the supplied precision. Your fix will compute the precision in all
cases, which would be a performance overhead. It should be computing only when supplied precision
is 0. Thoughts ? —
    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