> On 2011-03-16 16:37:44, Ning Zhang wrote: > > trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionPruner.java, > > line 244 > > <https://reviews.apache.org/r/489/diff/1/?file=13887#file13887line244> > > > > I agree in general we should use a generic interface in the > > declaration, but I'd prefer leave the LinkedHashMap here. The reason is > > that what we need for the partSpec is an ordered map where the order of > > iterator.getNext() should be the same order of elements being inserted. > > Unfortunately Java collections doesn't have this interface but just an > > implementation. We could declare an interface just for that, but that > > should be a different issue. > > M IS wrote: > Using the generic interface (java.util.Map in this context) in > declaration will not affect the expected FIFO functionality. > Further, when this map instance is being passed around to get the > partition in the getPartition(...) method, the reference type is generic Map > and not implementation type. > Here is a sample program to illustrate that using Map as reference for an > LinkedHashMap maintains the order: > > public class Sample { > public static void main(String[] args) { > Map<String, String> map = new LinkedHashMap<String, String>(); > > map.put("A", "A"); > map.put("B", "B"); > map.put("C", "C"); > map.put("D", "D"); > map.put("E", "E"); > map.put("F", "F"); > map.put("G", "G"); > > Iterator<String> iter = map.keySet().iterator(); > > while (iter.hasNext()) { > System.out.println(map.get(iter.next())); > } > > for (Map.Entry<String, String> entry : map.entrySet()) { > System.out.println(entry.getKey() + " " + > entry.getValue()); > } > } > }
I think I didn't make myself clear here. I know if the variable is declared as Map but the implementation is LinkedHashMap, it will preserve the order property. My concern is that if people only look at the declaration of partSpec without look deep inside the implementation of Warehouse.makePartName(), they only know it is a Map type. People may think they can do a set of operations (e.g., inserts) to this data structure in arbitrary order because order is not important/guaranteed in Map. It is easier to introduce bugs. If we declare it as LinkedHashMap, it is a reminder to people to think twice about the order of operations to this data structure. - Ning ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/489/#review333 ----------------------------------------------------------- On 2011-03-16 16:50:15, Ning Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/489/ > ----------------------------------------------------------- > > (Updated 2011-03-16 16:50:15) > > > Review request for hive. > > > Summary > ------- > > * expose HiveMetaStoreClient.listPartitionsByFilter() to Hive.java so > that PartitionPruner can use that for certain partition predicates. > * only allows {=, AND, OR} in the partition predicates that can be pushed > down to JDO filtering. > > > Diffs > ----- > > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java > 1081948 > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java > 1081948 > > trunk/metastore/src/java/org/apache/hadoop/hive/metastore/parser/ExpressionTree.java > 1081948 > trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java 1081948 > trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java 1081948 > > trunk/ql/src/java/org/apache/hadoop/hive/ql/optimizer/ppr/PartitionPruner.java > 1081948 > trunk/ql/src/test/queries/clientpositive/ppr_pushdown.q PRE-CREATION > trunk/ql/src/test/queries/clientpositive/ppr_pushdown2.q PRE-CREATION > trunk/ql/src/test/results/clientpositive/ppr_pushdown.q.out PRE-CREATION > trunk/ql/src/test/results/clientpositive/ppr_pushdown2.q.out PRE-CREATION > > Diff: https://reviews.apache.org/r/489/diff > > > Testing > ------- > > > Thanks, > > Ning > >
