Return-Path: X-Original-To: apmail-hive-dev-archive@www.apache.org Delivered-To: apmail-hive-dev-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 93ECD10652 for ; Sun, 6 Apr 2014 04:32:51 +0000 (UTC) Received: (qmail 49072 invoked by uid 500); 6 Apr 2014 04:32:50 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 48354 invoked by uid 500); 6 Apr 2014 04:32:47 -0000 Mailing-List: contact dev-help@hive.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@hive.apache.org Delivered-To: mailing list dev@hive.apache.org Received: (qmail 48339 invoked by uid 99); 6 Apr 2014 04:32:43 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 06 Apr 2014 04:32:43 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 7EDFE1D5D17; Sun, 6 Apr 2014 04:32:39 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============0713514568039654773==" MIME-Version: 1.0 Subject: Re: Review Request 18179: Support more generic way of using composite key for HBaseHandler From: "Xuefu Zhang" To: "Navis Ryu" , "Xuefu Zhang" , "hive" Date: Sun, 06 Apr 2014 04:32:39 -0000 Message-ID: <20140406043239.30548.23894@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org Auto-Submitted: auto-generated Sender: "Xuefu Zhang" X-ReviewGroup: hive X-ReviewRequest-URL: https://reviews.apache.org/r/18179/ X-Sender: "Xuefu Zhang" References: <20140402062908.2770.65524@reviews.apache.org> In-Reply-To: <20140402062908.2770.65524@reviews.apache.org> Reply-To: "Xuefu Zhang" X-ReviewRequest-Repository: hive-git --===============0713514568039654773== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- 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 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 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 Can we pass those as constructor arguments instead of individual set methods? ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java Is this to fix some bug? ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java What is this change for? ql/src/java/org/apache/hadoop/hive/ql/index/IndexPredicateAnalyzer.java There seems to have some indention problem. ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java Can we have some comments here describing what we are doing? ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeDescUtils.java Same as above. ql/src/java/org/apache/hadoop/hive/ql/plan/TableScanDesc.java I don't quite follow the need of serializing filter object. serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyObjectBase.java The class name doesn't sound like an interface. serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazySimpleSerDe.java 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 > > --===============0713514568039654773==--