[
https://issues.apache.org/jira/browse/HIVE-5206?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13764618#comment-13764618
]
Phabricator commented on HIVE-5206:
-----------------------------------
ashutoshc has requested changes to the revision "HIVE-5206 [jira] Support
parameterized primitive types".
Awesome comments. Thanks for taking time to put in informative comments!
INLINE COMMENTS
serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorFactory.java:215
I think its better that this method just takes primitiveInfo as an input,
because both category and typeParams can be derived from it. Advantage of that
is instead of callers providing two arguments, they can just pass in one.
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java:776 This
utility method should be in TypeInfoUtilities class, not in Function Registry.
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java:1279 Can't
this genericUDF implement SettableUDF ?
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java:1283 Are
macros not suppose to implement SettableUDF?
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java:1298 I think
this is an error condition, we can't swallow the exception here.
ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java:1273 In a
follow-up jira we should consider cloning object using Kryo, instead of this
reflection based approach. Since Kryo does field base serialization, it is much
more faithful in reproducing the state of object than this reflection based
approach.
ql/src/java/org/apache/hadoop/hive/ql/exec/SettableUDF.java:18 This interface
belongs in o.a.h.ql.udf
ql/src/java/org/apache/hadoop/hive/ql/exec/SettableUDF.java:23 We need to
explicitly annotate this interface to be private, unstable.
ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java:5332
Instead of increasing the size of SemanticAnalyzer, this really belongs to
ParseUtils.
ql/src/java/org/apache/hadoop/hive/ql/parse/TypeCheckProcFactory.java:647
This could just be package private instead of public.
serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/primitive/PrimitiveObjectInspectorFactory.java:313
As stated in previous comment, this could just take in PrimitiveTypeEntry as
one parameter.
serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/primitive/LazyPrimitiveObjectInspectorFactory.java:144
TypeInfo contains both category as well as typeParams, so consider passing
just that as an argument.
serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/PrimitiveTypeInfo.java:99
Now with Kryo, does that improve things ?
serde/src/java/org/apache/hadoop/hive/serde2/typeinfo/TypeInfoFactory.java:64
Not sure if we can swallow exception here or we need to throw. Can you add a
comment why is it ok to do so?
REVISION DETAIL
https://reviews.facebook.net/D12693
BRANCH
HIVE-5206
ARCANIST PROJECT
hive
To: JIRA, ashutoshc, jdere
> Support parameterized primitive types
> -------------------------------------
>
> Key: HIVE-5206
> URL: https://issues.apache.org/jira/browse/HIVE-5206
> Project: Hive
> Issue Type: Improvement
> Components: Types
> Reporter: Jason Dere
> Assignee: Jason Dere
> Attachments: HIVE-5206.1.patch, HIVE-5206.2.patch,
> HIVE-5206.D12693.1.patch
>
>
> Support for parameterized types is needed for char/varchar/decimal support.
> This adds a type parameters value to the
> PrimitiveTypeEntry/PrimitiveTypeInfo/PrimitiveObjectInspector objects.
> NO PRECOMMIT TESTS - dependent on HIVE-5203/HIVE-5204
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
