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 Sun, 06 Apr 2014 04:32:39 GMT

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


Patch looks good, except that I'm not quite sure if I fully understand all the functionality
introduced. Some comments will be certainly helpful.

I saw quite some new code that I didn't see previously. Maybe I missed, but it's good that
we limit the scope of changes. If certain changes are not absolutely needed for a JIRA, we
can always file followup JIRAs. A big patch is much hard to be reviewed than a few small patches.


hbase-handler/src/java/org/apache/hadoop/hive/hbase/ColumnMappings.java
<https://reviews.apache.org/r/18179/#comment72094>

    I think I had a previous comment that hasn't been addressed. It' scopied here:
    
    Can we use List? I don't see any advantages of array in this case. Rather, it brings additional
code handling the list-array conversion.



hbase-handler/src/java/org/apache/hadoop/hive/hbase/CompositeHBaseKeyFactory.java
<https://reviews.apache.org/r/18179/#comment72210>

    Can we have come comments describe what we achieve here functionally? It's hard to understand
what's going on here.
    
    I didn't see this part of code in previous review.



hbase-handler/src/java/org/apache/hadoop/hive/hbase/CompositeHBaseKeyFactory.java
<https://reviews.apache.org/r/18179/#comment72209>

    Can we pass those as constructor arguments instead of individual set methods?



ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java
<https://reviews.apache.org/r/18179/#comment72095>

    Is this to fix some bug?



ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java
<https://reviews.apache.org/r/18179/#comment72097>

    What is this change for?



ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java
<https://reviews.apache.org/r/18179/#comment72099>

    There seems to have some indention problem.



ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java
<https://reviews.apache.org/r/18179/#comment72205>

    Can we have some comments here describing what we are doing?



ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java
<https://reviews.apache.org/r/18179/#comment72206>

    Same as above.



ql/src/java/org/apache/hadoop/hive/ql/plan/TableScanDesc.java
<https://reviews.apache.org/r/18179/#comment72103>

    I don't quite follow the need of serializing filter object.



serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyObjectBase.java
<https://reviews.apache.org/r/18179/#comment72207>

    The class name doesn't sound like an interface.



serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazySimpleSerDe.java
<https://reviews.apache.org/r/18179/#comment72208>

    I don't see any use of this method.


- Xuefu Zhang


On April 2, 2014, 6:28 a.m., Navis Ryu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18179/
> -----------------------------------------------------------
> 
> (Updated April 2, 2014, 6:28 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/AbstractHBaseKeyFactory.java PRE-CREATION

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

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

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

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseCompositeKey.java 5008f15

>   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/HBaseRowSerializer.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 5fe35a5 
>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseSerDeParameters.java b64590d

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HBaseStorageHandler.java 4fe1b1b

>   hbase-handler/src/java/org/apache/hadoop/hive/hbase/HiveHBaseTableInputFormat.java
142bfd8 
>   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/org/apache/hadoop/hive/hbase/TestLazyHBaseObject.java 7c4fc9f

>   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 e52d364 
>   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 f0c0ecf 
>   ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcStruct.java 293b74e 
>   ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ArrayWritableObjectInspector.java
bb02bab 
>   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 ecb82d7 
>   ql/src/java/org/apache/hadoop/hive/ql/ppd/OpProcFactory.java c0a8269 
>   serde/src/java/org/apache/hadoop/hive/serde2/BaseStructObjectInspector.java PRE-CREATION

>   serde/src/java/org/apache/hadoop/hive/serde2/NullStructSerDe.java dba5e33 
>   serde/src/java/org/apache/hadoop/hive/serde2/StructObject.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/LazySimpleSerDe.java 82c1263 
>   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

>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ColumnarStructObjectInspector.java
7d0d91c 
>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/DelegatedStructObjectInspector.java
5e1a369 
>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/ReflectionStructObjectInspector.java
bd3cdd4 
>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/StructField.java 67827d6

>   serde/src/java/org/apache/hadoop/hive/serde2/objectinspector/UnionStructObjectInspector.java
60e55ec 
> 
> Diff: https://reviews.apache.org/r/18179/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Navis Ryu
> 
>


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