[
https://issues.apache.org/jira/browse/AVRO-1877?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15369995#comment-15369995
]
Yibing Shi edited comment on AVRO-1877 at 7/10/16 11:29 PM:
------------------------------------------------------------
{quote}
Honestly, I like the old code better.
{quote}
The problem with the old code is that it checks for conversions twice: once in
{{javaUnbox}}, once again in {{javaType}}
That said, I have a look at the code again and the extra test for UNION does
smells bad. I will refactor the code a bit more and upload another patch here.
{quote}
As for enableDecimalLogicalType. If truly the intent is to use converted
logical types at all, then this should be renamed to enableLogicalType. It's
confusing to say it's for Decimal but mean it for all.
{quote}
As said before, {{enableDecimalLogicalType}} should only applies to decimal
type, not the date/time logical types, which should always use Joda/Java
date/time classes instead of raw types if these classes are present.
Please have a look at the unit tests added in {{TestSpecificCompiler}}
(testJavaTypeWithDecimalLogicalTypeDisabled/Enabled). They clearly show the
intent of {{enableDecimalLogicalType}}
{quote}
More importantly, after applying your AVRO-1847, if you have date or
timestamp-millis without setting enableDecmialLogicalType, the compiled
SpecificRecord code will NOT compile.
{quote}
This problem is caused because the problem tracked in this JIRA (AVRO-1877).
Please try apply AVRO-1877 on top of AVRO-1847 and see whether it fixes the
problem for you.
was (Author: yibing):
{quote}
Honestly, I like the old code better.
{quote}
The problem with the old code is that it checks for conversions twice: once in
{{javaUnbox}}, once again in {{javaType}}
That said, I have a look at the code again and the extra test for UNION does
smells bad. I will refactor the code a bit more and upload another patch here.
{quote}
As for enableDecimalLogicalType. If truly the intent is to use converted
logical types at all, then this should be renamed to enableLogicalType. It's
confusing to say it's for Decimal but mean it for all.
{quote}
As said before, {{enableDecimalLogicalType}} should only applies to decimal
type, not the date/time logical types, which should always use Joda/Java
date/time classes instead of raw types if these classes are present.
{quote}
More importantly, after applying your AVRO-1847, if you have date or
timestamp-millis without setting enableDecmialLogicalType, the compiled
SpecificRecord code will NOT compile.
{quote}
This problem is caused because the problem tracked in this JIRA (AVRO-1877).
Please try apply AVRO-1877 on top of AVRO-1847 and see whether it fixes the
problem for you.
> AVRO-1847 accidentally breaks javaUnbox method in SpecificCompiler
> ------------------------------------------------------------------
>
> Key: AVRO-1877
> URL: https://issues.apache.org/jira/browse/AVRO-1877
> Project: Avro
> Issue Type: Bug
> Components: java
> Affects Versions: 1.9.0
> Reporter: Yibing Shi
> Priority: Blocker
> Attachments: AVRO-1877.1.patch, AVRO-1877.2.patch
>
>
> AVRO-1847 accidentally removes the logical type conversion logic in method
> {{SpecificCompiler.javaUnbox}}, which breaks the data time type compiling in
> specific compiler.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
