hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Navis Ryu" <navis....@nexr.com>
Subject Re: Review Request 18179: Support more generic way of using composite key for HBaseHandler
Date Mon, 07 Apr 2014 07:47:52 GMT


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > hbase-handler/src/java/org/apache/hadoop/hive/hbase/CompositeHBaseKeyFactory.java,
line 98
> > <https://reviews.apache.org/r/18179/diff/8/?file=544997#file544997line98>
> >
> >     Can we pass those as constructor arguments instead of individual set methods?

It was like that long before and not fixed even in your refactoring patch. I cannot sure it's
meaningful to change those columns to be arguments of constructor, if the analyzer has add
method. Or you mean we should remove add method altogether?


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java, line 212
> > <https://reviews.apache.org/r/18179/diff/8/?file=545019#file545019line212>
> >
> >     What is this change for?

Currently, IndexSearchCondition does not allow field, so I've improvised string[] to describe
fields not hurting other codes. Wish I could have a better idea.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java, line 264
> > <https://reviews.apache.org/r/18179/diff/8/?file=545025#file545025line264>
> >
> >     Can we have some comments here describing what we are doing?

sure


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java, line 277
> > <https://reviews.apache.org/r/18179/diff/8/?file=545025#file545025line277>
> >
> >     Same as above.

sure


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/plan/TableScanDesc.java, line 73
> > <https://reviews.apache.org/r/18179/diff/8/?file=545026#file545026line73>
> >
> >     I don't quite follow the need of serializing filter object.

In the HIVE-6290, analyzer makes a Scan object rather than ExprDesc. So made generic way of
handling this kind of special predicates. I used similar way for handling over predicate object
into non-native system and felt it's quite useful.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazySimpleSerDe.java, line 174
> > <https://reviews.apache.org/r/18179/diff/8/?file=545034#file545034line174>
> >
> >     I don't see any use of this method.

Remnant of previous patch. removed.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > hbase-handler/src/java/org/apache/hadoop/hive/hbase/ColumnMappings.java, line 40
> > <https://reviews.apache.org/r/18179/diff/8/?file=544996#file544996line40>
> >
> >     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.

commented above as 

"I prefer arrays than lists for non-mutables, which implies it's ready and would not be added
or removed further"

Yes, I just prefer array. If you don't like it, I'll change.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > hbase-handler/src/java/org/apache/hadoop/hive/hbase/CompositeHBaseKeyFactory.java,
line 93
> > <https://reviews.apache.org/r/18179/diff/8/?file=544997#file544997line93>
> >
> >     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.

Much of codes are included from the patch of HIVE-6290, cause I didn't bother Swarnim to rebase
it on this. I should do some investigation for proper comments on it.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java, line 190
> > <https://reviews.apache.org/r/18179/diff/8/?file=545019#file545019line190>
> >
> >     Is this to fix some bug?

Regretfully, I've appended null marker to discern const-column between column-const in HIVE-3617.
That became more complicated by supporting ExprNodeFieldDesc to be analyzed. Now we know the
order by comparing the first ExprDesc of array is constant.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java, line 281
> > <https://reviews.apache.org/r/18179/diff/8/?file=545019#file545019line281>
> >
> >     There seems to have some indention problem.

You mean 100 columns limit? I thought anyone respects code convention in theses days. I'll
fix that.


> On April 6, 2014, 4:32 a.m., Xuefu Zhang wrote:
> > serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyObjectBase.java, line 21
> > <https://reviews.apache.org/r/18179/diff/8/?file=545033#file545033line21>
> >
> >     The class name doesn't sound like an interface.

When this be done, users would implement or extend this class. From the aspect of freedom
provided, I thought interface would be (much) better for them. Wanted the name 'LazyObject',
but it's already occupied and couldn't find excuses for further changes.


- Navis


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


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