Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============5647982807684925708=="
MIME-Version: 1.0
Subject: Re: Review Request 18925: HIVE-6575 select * fails on parquet table
 with map
 datatype
From: "Xuefu Zhang" <xzhang@cloudera.com>
To: "justin coffey" <j.coffey@criteo.com>,
 "Xuefu Zhang" <xzhang@cloudera.com>,
 "Brock Noland" <brock@cloudera.com>
Cc: "Szehon Ho" <szehon@cloudera.com>, "hive" <dev@hive.apache.org>
Date: Fri, 07 Mar 2014 23:06:45 -0000
Message-ID: <20140307230645.2625.40999@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Xuefu Zhang" <noreply@reviews.apache.org>
References: <20140307222011.2625.4474@reviews.apache.org>
In-Reply-To: <20140307222011.2625.4474@reviews.apache.org>
Reply-To: "Xuefu Zhang" <xzhang@cloudera.com>

--===============5647982807684925708==
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/18925/#review36578
-----------------------------------------------------------


ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java
<https://reviews.apache.org/r/18925/#comment67584>

    I don't think we should check the class equality here. A child class might inherit a parent class w/o any new identifier. In that case the child class can still be equal to the parent class. It's really up to the child class to decide the equality (by overriding the equal()).


ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java
<https://reviews.apache.org/r/18925/#comment67585>

    1. code style: we may need {} for else if block
    
    2. However, I think this might be refactored as follows for easier understanding:
    
    boolean keyEq = (keyInspector == null && other.keyInspector == null) || keyInspector.equal(other.keyInspector);
    boolean valEq = (ValueInspector == null && other.valueInspector == null) || valueInspector.equal(other.valueInspector);
    return keyEq && valEq;


- Xuefu Zhang


On March 7, 2014, 10:20 p.m., Szehon Ho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18925/
> -----------------------------------------------------------
> 
> (Updated March 7, 2014, 10:20 p.m.)
> 
> 
> Review request for hive, Brock Noland, justin coffey, and Xuefu Zhang.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The issue is, as part of select * query, a DeepParquetHiveMapInspector is used for one column of an overall parquet-table struct object inspector.  
> 
> The problem lies in the ObjectInspectorFactory's cache for struct object inspector.  For performance, there is a cache keyed on an array list, of all object inspectors of columns.  The second time the query is run, it attempts to lookup cached struct inspector.  But when the hashmap looks up the part of the key consisting of the DeepParquetHiveMapInspector, java calls .equals against the existing DeepParquetHivemapInspector.  This fails, as the .equals method casted the "other" to a "StandardParquetHiveInspector".
> 
> Regenerating the .equals and .hashcode from eclipse.  
> 
> Also adding one more check in .equals before casting, to handle the case if another class of object inspector gets hashed to the same hashcode in the cache.  Then java would call .equals against the other, which in this case is not of the same class.
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/io/parquet/serde/AbstractParquetMapInspector.java 1d72747 
> 
> Diff: https://reviews.apache.org/r/18925/diff/
> 
> 
> Testing
> -------
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Szehon Ho
> 
>


--===============5647982807684925708==--