[GitHub] drill issue #517: DRILL-4704 fix

[daveoshinsky]

Github user daveoshinsky commented on the issue:

    https://github.com/apache/drill/pull/517
  
    No, negative precision will not work properly.  I committed another change 
to PR-517 to handle this.
    Quite honestly, I do not know how to fix the problem as you suggest in 
ExpressionTreeMaterializer.java.  First, I set a breakpoint on line 315, and 
it never fired.  It went to the breakpoint on line 172, never passing line 315 
(where I set a breakpoint).  I see that code at line 172 is making a call to a 
virtual function toType.getPrecision() to obtain the precision, from an object 
of a type that is only known at runtime.  Based on what I saw in the debugger, 
the type of the "toType" object at line 172 was something like 
TypeProtos$MajorType@8570, which looked like a piece of runtime generated code 
(as often seen in Drill), and the debugger would not let me single step into 
it.  That toType object did have both scale and precision = 0.  Now, if I 
could obtain the integer value of the toType object, I could calculate the 
precision right there (if the getPrecision() returns 0).  But, I don't see how 
to get the integer value at line 172.  In fact, I'm guessing that "getting 
 an integer value" only applies in some cases, for some kinds of values that 
are being "materialized".
    To summarize, I have not been looking at Apache Drill nearly as long as you 
have, and don't understand it nearly as well as you.  Yet, you are asking me 
to change a fix that already works, to fit a model you appear to have in mind 
for a fix as you would do it.  I simply don't have the level of understanding 
of the Drill code to do the fix as you imagine it should be done.  Moreover, I 
am a volunteer, being paid by my employer to work mainly on things other than 
Apache Drill.  As such, the amount of additional time I can spend on this fix 
is limited.  My fix already works AFAIK, and is not inefficient.  I suggest 
you take it, and if necessary, then change it to your liking.  The unit test 
I've included is a decent test of the DRILL-4704 problem. 
    
        On Thursday, August 4, 2016 4:06 PM, Jinfeng Ni 
<notificati...@github.com> wrote:
     
    
     
    I attached debugger to Drill and found it's passing through line 172 (not 
315-316) of ExpressionTreeMaterializer.java, and that the "toType" object is 
one of those runtime generated ones (name like TypeProtos$MajorType@8570). 
Rather than attempt to change that code, I made another change to 
CastIntDecimal.java which avoids repeated calculation of the precision when it 
is already provided. Please review.
    Line 172 was called after scale/precision are set on line 315-316.I 
understand your new change will kick in when the passed precision= 0. However, 
I still think that the right way is to pass in the correct scale/precision when 
call that cast function, in stead of figuring out on-the-fly in the middle of 
that function. If someone pass a 0 precision (which is not right), we need fix 
that in the place where 0 precision is specified.Also, will your code work 
correct for negative integer input?—
    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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---