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 08FE210413 for ; Tue, 4 Feb 2014 22:21:18 +0000 (UTC) Received: (qmail 51070 invoked by uid 500); 4 Feb 2014 22:21:16 -0000 Delivered-To: apmail-hive-dev-archive@hive.apache.org Received: (qmail 50950 invoked by uid 500); 4 Feb 2014 22:21:15 -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 50939 invoked by uid 99); 4 Feb 2014 22:21:15 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 04 Feb 2014 22:21:15 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 2D7231D4763; Tue, 4 Feb 2014 22:21:15 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============3869423238348519333==" MIME-Version: 1.0 Subject: Re: Review Request 17061: HIVE-5783 - Native Parquet Support in Hive From: "Xuefu Zhang" To: "Vijayakumar Ramdoss" , "Xuefu Zhang" , "hive" , "Brock Noland" Date: Tue, 04 Feb 2014 22:21:14 -0000 Message-ID: <20140204222114.26209.5938@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/17061/ X-Sender: "Xuefu Zhang" References: <20140204214428.14639.6975@reviews.apache.org> In-Reply-To: <20140204214428.14639.6975@reviews.apache.org> Reply-To: "Xuefu Zhang" X-ReviewRequest-Repository: hive-git --===============3869423238348519333== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit > On Feb. 4, 2014, 9:44 p.m., Xuefu Zhang wrote: > > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ArrayWritableGroupConverter.java, line 42 > > > > > > I'd rather throw an exception if (count != 1 && count != 2) then using assert here. > > Brock Noland wrote: > Maybe I misunderstood your earlier comment about this code "The if ... else ... here doesn't seem terribly different. Please refeactor the code." as the earlier code seems like what you are suggesting. Can you psuedo code what you'd like to see here? > > Sorry for the confusion. My earlier comments meant to say that the two cases, count = 1 or count = 2, seemed very similar, except setting isMap either true or false.Thus, it seemed that the two block could be combined. int count = groupType.getFieldCount(); if (count != 1 && count != 2) { throw new Exception .... } isMap = count == 2; converters = new Converter[count]; for (int i = 0; i < count; i++) { converters[i] = getConverterFromDescription(groupType.getType(count - 1), i, this); } Now I realize that the code is simpler yet a little harder to understand. So, it's up to you. However, I think throwing an exception is better than an assertion regardless. - Xuefu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17061/#review33638 ----------------------------------------------------------- On Feb. 4, 2014, 8:29 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17061/ > ----------------------------------------------------------- > > (Updated Feb. 4, 2014, 8:29 p.m.) > > > Review request for hive. > > > Bugs: HIVE-5783 > https://issues.apache.org/jira/browse/HIVE-5783 > > > Repository: hive-git > > > Description > ------- > > Adds native parquet support hive > > > Diffs > ----- > > data/files/parquet_create.txt PRE-CREATION > data/files/parquet_partitioned.txt PRE-CREATION > pom.xml 41f5337 > ql/pom.xml 7087a4c > ql/src/java/org/apache/hadoop/hive/ql/io/IOConstants.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetInputFormat.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/MapredParquetOutputFormat.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/ProjectionPusher.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ArrayWritableGroupConverter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/DataWritableGroupConverter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/DataWritableRecordConverter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/ETypeConverter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveGroupConverter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/convert/HiveSchemaConverter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/DataWritableReadSupport.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/read/ParquetRecordReaderWrapper.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ArrayWritableObjectInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/DeepParquetHiveMapInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveArrayInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/ParquetHiveSerDe.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/StandardParquetHiveMapInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetByteInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetPrimitiveInspectorFactory.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetShortInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/primitive/ParquetStringInspector.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/writable/BigDecimalWritable.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/writable/BinaryWritable.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriteSupport.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/DataWritableWriter.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/io/parquet/write/ParquetRecordWriterWrapper.java PRE-CREATION > ql/src/java/org/apache/hadoop/hive/ql/metadata/VirtualColumn.java 2bc7e86 > ql/src/java/org/apache/hadoop/hive/ql/parse/BaseSemanticAnalyzer.java 13d0a56 > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveLexer.g f83c15d > ql/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g 17f3552 > ql/src/java/org/apache/hadoop/hive/ql/parse/IdentifiersParser.g 538b2b0 > ql/src/java/org/apache/hadoop/hive/ql/parse/SemanticAnalyzer.java 719b496 > ql/src/java/org/apache/hadoop/hive/ql/parse/authorization/HiveAuthorizationTaskFactoryImpl.java 4ac8f07 > ql/src/java/parquet/hive/DeprecatedParquetInputFormat.java PRE-CREATION > ql/src/java/parquet/hive/DeprecatedParquetOutputFormat.java PRE-CREATION > ql/src/java/parquet/hive/MapredParquetInputFormat.java PRE-CREATION > ql/src/java/parquet/hive/MapredParquetOutputFormat.java PRE-CREATION > ql/src/java/parquet/hive/serde/ParquetHiveSerDe.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestHiveSchemaConverter.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestMapredParquetInputFormat.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestMapredParquetOutputFormat.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/TestParquetSerDe.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestAbstractParquetMapInspector.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestDeepParquetHiveMapInspector.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestParquetHiveArrayInspector.java PRE-CREATION > ql/src/test/org/apache/hadoop/hive/ql/io/parquet/serde/TestStandardParquetHiveMapInspector.java PRE-CREATION > ql/src/test/queries/clientpositive/parquet_create.q PRE-CREATION > ql/src/test/queries/clientpositive/parquet_partitioned.q PRE-CREATION > ql/src/test/results/clientnegative/authorization_invalid_priv_v1.q.out 431f16e > ql/src/test/results/clientpositive/parquet_create.q.out PRE-CREATION > ql/src/test/results/clientpositive/parquet_partitioned.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/17061/diff/ > > > Testing > ------- > > > Thanks, > > Brock Noland > > --===============3869423238348519333==--