> 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; > > Xuefu Zhang wrote: > 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. > > Navis Ryu wrote: > I've tried but we needs > > Object f, ObjectInspector foi, StructField declared, ColumnMapping > colMap, SerDeParameters serdeParams, ByteStream.Output serializeStream > > If we hand over HBaseSerde, > > Object f, ObjectInspector foi, StructField declared, ColumnMapping > colMap, HBaseSerde serde > > Should we really do this? it aches my belly.
If we're not ready for this, I think we should remove HBaseWritableKeyFactory interface. We can add the serialize() interface in the future when we are have a better idea. - 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 > >
