[ https://issues.apache.org/jira/browse/DRILL-4834?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16321036#comment-16321036 ]
ASF GitHub Bot commented on DRILL-4834: --------------------------------------- Github user daveoshinsky commented on a diff in the pull request: https://github.com/apache/drill/pull/570#discussion_r160790684 --- Diff: exec/vector/src/main/codegen/templates/ComplexWriters.java --- @@ -99,7 +99,7 @@ public void write(Nullable${minor.class?cap_first}Holder h) { <#if !(minor.class == "Decimal9" || minor.class == "Decimal18" || minor.class == "Decimal28Sparse" || minor.class == "Decimal38Sparse" || minor.class == "Decimal28Dense" || minor.class == "Decimal38Dense")> public void write${minor.class}(<#list fields as field>${field.type} ${field.name}<#if field_has_next>, </#if></#list>) { - mutator.addSafe(idx(), <#list fields as field>${field.name}<#if field_has_next>, </#if></#list>); + mutator.addSafe(idx(), <#list fields as field><#if field.name == "scale"><#break></#if>${field.name}<#if field_has_next && fields[field_index+1].name != "scale" >, </#if></#list>); --- End diff -- I had a hard time understanding how this codegen stuff works, so some of the code I wrote was due to my struggling to understand how it is SUPPOSED to work. As such, I found that it needed to "break" out of code generation just after the "scale" argument, to avoid compilation failure of the generated code (IIRC, it was quite a while ago), and that is why I coded it this way. What exact change are you suggesting? To AND the field.name check=="scale" check with a check that minor.class=="VarDecimal"? > decimal implementation is vulnerable to overflow errors, and extremely complex > ------------------------------------------------------------------------------ > > Key: DRILL-4834 > URL: https://issues.apache.org/jira/browse/DRILL-4834 > Project: Apache Drill > Issue Type: Bug > Components: Execution - Data Types > Affects Versions: 1.6.0 > Environment: Drill 1.7 on any platform > Reporter: Dave Oshinsky > Fix For: Future > > > While working on a fix for DRILL-4704, logic was added to CastIntDecimal.java > template to handle the situation where a precision is not supplied (i.e., the > supplied precision is zero) for an integer value that is to be casted to a > decimal. The Drill decimal implementation uses a limited selection of fixed > decimal precision data types (the total number of decimal digits, i.e., > Decimal9, 18, 28, 38) to represent decimal values. If the destination > precision is too small to represent the input integer that is being casted, > there is no clean way to deal with the overflow error properly. > While using fixed decimal precisions as is being done currently can lead to > more efficient use of memory, it often will actually lead to less efficient > use of memory (when the fixed precision is specified significantly larger > than is actually needed to represent the numbers), and it results in a > tremendous mushrooming of the complexity of the code. For each fixed > precision (and there are only a limited set of selections, 9, 18, 28, 38, > which itself leads to memory inefficiency), there is a separate set of code > generated from templates. For each pairwise combination of decimal or > non-decimal numeric types, there are multiple places in the code where > conversions must be handled, or conditions must be included to handle the > difference in precision between the two types. A one-size-fits-all approach > (using a variable width vector to represent any decimal precision) would > usually be more memory-efficient (since precisions are often over-specified), > and would greatly simplify the code. > Also see the DRILL-4184 issue, which is related. -- This message was sent by Atlassian JIRA (v6.4.14#64029)