hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xuefu Zhang" <xzh...@cloudera.com>
Subject Re: Review Request 18179: Support more generic way of using composite key for HBaseHandler
Date Tue, 11 Mar 2014 17:41:21 GMT


> On March 10, 2014, 9:25 p.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java, line 702
> > <https://reviews.apache.org/r/18179/diff/5/?file=513405#file513405line702>
> >
> >     Do these methods have to be public? Private if just used locally.
> 
> Navis Ryu wrote:
>     Seemed to find use case for this in sometime. But ok.

We can make them public when the use case comes, but not the other way around.


> On March 10, 2014, 9:25 p.m., Xuefu Zhang wrote:
> > hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDe.java, line 730
> > <https://reviews.apache.org/r/18179/diff/5/?file=513392#file513392line730>
> >
> >     Can we define serielize() interface in HBaseKeyFactory, move the existing implementation
here to HBaseCompositeKeyFactory? Serialize() seems seems generic enough to expect from all
key factories. Doing this will eliminate HBaseWritableKeyFactory and use of the class to detect
what method to call.
> 
> Navis Ryu wrote:
>     If the default serialization can be done by simple decent method call, I would have
done like that. But current implementation needs seven argument for that(+serdeParams), which
made me think twice of it. 
>     
>     byte[] serialize(
>         int i,
>         List<ColumnMapping> mapping,
>         List<? extends StructField> fields,
>         List<Object> list,
>         List<? extends StructField> declaredFields,
>         boolean useJSONSerialize,
>         ByteStream.Output serializeStream) throws IOException;

Yes, I agree that too many params for a method is ugly. In this case, however, it doesn't
seem too bad:

1. i and and the 4 lists can be reduced to 4 fields, as i is just the index in the lists,
which are derived from object inspector and serdeparams. To further reduce the arg number,
a struct can be defined to wrap the 4 items: keyMapping, keyField, keyObject, and keyDeclaredField.
(keyDeclaredField may not be needed as we are talking about row key here.)

2. useJsonSerialize seems always false, so it can be removed.

I understand that some refactoring is needed. However, I think it's worth the effort for readability
and maintenance.


- Xuefu


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18179/#review36688
-----------------------------------------------------------


On March 7, 2014, 7:46 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18179/
> -----------------------------------------------------------
> 
> (Updated March 7, 2014, 7:46 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Bugs: HIVE-6411
>     https://issues.apache.org/jira/browse/HIVE-6411
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> HIVE-2599 introduced using custom object for the row key. But it forces key objects to
extend HBaseCompositeKey, which is again extension of LazyStruct. If user provides proper
Object and OI, we can replace internal key and keyOI with those. 
> 
> Initial implementation is based on factory interface.
> {code}
> public interface HBaseKeyFactory {
>   void init(SerDeParameters parameters, Properties properties) throws SerDeException;
>   ObjectInspector createObjectInspector(TypeInfo type) throws SerDeException;
>   LazyObjectBase createObject(ObjectInspector inspector) throws SerDeException;
> }
> {code}
> 
> 
> Diffs
> -----
> 
>   hbase-handler/pom.xml 132af43 
>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseCompositeKey.java 5008f15

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseCompositeKeyFactory.java PRE-CREATION

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseKeyFactory.java PRE-CREATION

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseLazyObjectFactory.java PRE-CREATION

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseScanRange.java PRE-CREATION

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDe.java 2cd65cb 
>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java 29e5da5

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseWritableKeyFactory.java PRE-CREATION

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableInputFormat.java
704fcb9 
>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/LazyHBaseRow.java fc40195 
>   hbase-handler/src/test/org/apache/hadoop/hive/hbase/HBaseTestCompositeKey.java 13c344b

>   hbase-handler/src/test/org/apache/hadoop/hive/hbase/TestHBaseKeyFactory.java PRE-CREATION

>   hbase-handler/src/test/org/apache/hadoop/hive/hbase/TestHBaseKeyFactory2.java PRE-CREATION

>   hbase-handler/src/test/queries/positive/hbase_custom_key.q PRE-CREATION 
>   hbase-handler/src/test/queries/positive/hbase_custom_key2.q PRE-CREATION 
>   hbase-handler/src/test/results/positive/hbase_custom_key.q.out PRE-CREATION 
>   hbase-handler/src/test/results/positive/hbase_custom_key2.q.out PRE-CREATION 
>   itests/util/pom.xml e9720df 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java b966d33 
>   ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java d39ee2e 
>   ql/src/java/org/apache/hadoop/hive/ql/index/IndexSearchCondition.java 5f1329c 
>   ql/src/java/org/apache/hadoop/hive/ql/io/HiveInputFormat.java 647a9a6 
>   ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveStoragePredicateHandler.java 9f35575

>   ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java e50026b 
>   ql/src/java/org/apache/hadoop/hive/ql/plan/TableScanDesc.java 10bae4d 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/OpProcFactory.java 40298e1 
>   serde/src/java/org/apache/hadoop/hive/serde2/StructObject.java PRE-CREATION 
>   serde/src/java/org/apache/hadoop/hive/serde2/StructObjectBaseInspector.java PRE-CREATION

>   serde/src/java/org/apache/hadoop/hive/serde2/columnar/ColumnarStructBase.java 1fd6853

>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyObject.java 10f4c05 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyObjectBase.java 3334dff 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyStruct.java 8a1ea46 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazy/objectinspector/LazySimpleStructObjectInspector.java
8a5386a 
>   serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryObject.java 598683f

>   serde/src/java/org/apache/hadoop/hive/serde2/lazybinary/LazyBinaryStruct.java caf3517

> 
> Diff: https://reviews.apache.org/r/18179/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message