[
https://issues.apache.org/jira/browse/HADOOP-3522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12605943#action_12605943
]
Hudson commented on HADOOP-3522:
--------------------------------
Integrated in Hadoop-trunk #522 (See
[http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/522/])
> ValuesIterator.next() doesn't return a new object, thus failing many equals()
> tests.
> ------------------------------------------------------------------------------------
>
> Key: HADOOP-3522
> URL: https://issues.apache.org/jira/browse/HADOOP-3522
> Project: Hadoop Core
> Issue Type: Bug
> Affects Versions: 0.17.0
> Reporter: Spyros Blanas
> Assignee: Owen O'Malley
> Fix For: 0.17.1
>
> Attachments: reduce-doc-19.patch, reduce-doc.patch
>
>
> h2.Problem
> The ValuesIterator.next() doesn't return a new object representing the next
> element.
> It is expected that this is the case. Example from java.lang.String source
> code:
> {noformat}
> public boolean equals(Object anObject) {
> if (this == anObject) {
> return true;
> }
> if (anObject instanceof String) {
> // custom code here
> }
> }
> {noformat}
> Changing the private fields of the object and returning it from next() will
> make this object fail the equals() test, because this==anObject will always
> be true.
> h2. What is affected
> The reduce() method presents (a subclass of) ValuesIterator to the user. So
> every user-defined class can be affected.
> Manifestations of this bug in 0.17.0:
> * The Text class checks for equality similarly to String.equals(), which is
> shown above.
> * The contrib/data_join breaks because it stores tags in a Map. The behavior
> of the next() method makes Object.equals() be true for all tags.
> h2. JUnit test
> Patch against hadoop-0.17.0:
> {noformat}
> Index: src/test/org/apache/hadoop/mapred/TestReduceTask.java
> ===================================================================
> --- src/test/org/apache/hadoop/mapred/TestReduceTask.java (revision
> 665935)
> +++ src/test/org/apache/hadoop/mapred/TestReduceTask.java (working copy)
> @@ -122,4 +122,52 @@
> runValueIterator(tmpDir, testCase, conf);
> }
> }
> +
> + public void runValueIterator2(Path tmpDir, Pair[] vals,
> + Configuration conf) throws IOException {
> + FileSystem fs = tmpDir.getFileSystem(conf);
> + Path path = new Path(tmpDir, "data.in");
> + SequenceFile.Writer writer = new SequenceFile.Writer(fs, conf, path,
> + Text.class,
> + Text.class);
> + for(Pair p: vals) {
> + writer.append(new Text(p.key), new Text(p.value));
> + }
> + writer.close();
> + SequenceFile.Sorter sorter = new SequenceFile.Sorter(fs, Text.class,
> + Text.class, conf);
> + SequenceFile.Sorter.RawKeyValueIterator rawItr =
> + sorter.merge(new Path[]{path}, false, tmpDir);
> + @SuppressWarnings("unchecked") // WritableComparators are not generic
> + ReduceTask.ValuesIterator<Text,Text> valItr =
> + new ReduceTask.ValuesIterator<Text,Text>(rawItr,
> + WritableComparator.get(Text.class), Text.class, Text.class,
> + conf, new NullProgress());
> + while (valItr.more()) {
> + Object key = valItr.getKey();
> + String keyString = key.toString();
> + // make sure it matches!
> + assertEquals(vals[0].key, keyString);
> + // must have at least 1 value!
> + assertTrue(valItr.hasNext());
> + Text value1 = valItr.next();
> + // must have at least 2 values!
> + assertTrue(valItr.hasNext());
> + Text value2 = valItr.next();
> + // do test
> + assertNotSame(value1, value2);
> + assertTrue(!value1.equals(value2));
> + assertTrue(!value2.equals(value1));
> + // make sure the key hasn't changed under the hood
> + assertEquals(keyString, valItr.getKey().toString());
> + valItr.nextKey();
> + }
> + }
> +
> + public void testValueIterator2() throws Exception {
> + Path tmpDir = new Path("build/test/test.reduce.task");
> + Configuration conf = new Configuration();
> + Pair[] test = new Pair[]{ new Pair("k1", "v1"), new Pair("k1", "v2") };
> + runValueIterator2(tmpDir, test, conf);
> + }
> }
> {noformat}
> h2. A program with a bug
> I would imagine that a programmer would be really confused when everything is
> equal in the example below, for any text input:
> {noformat}
> package test;
> import java.io.*;
> import java.util.*;
> import org.apache.hadoop.fs.Path;
> import org.apache.hadoop.conf.*;
> import org.apache.hadoop.io.*;
> import org.apache.hadoop.util.*;
> import org.apache.hadoop.mapred.*;
> import org.apache.hadoop.mapred.lib.*;
> public class TestNewReducer extends Configured implements Tool {
> public int run(String[] args) throws Exception {
> if (args.length < 2) {
> System.out.println("TestNewReducer <inDir> <outDir>");
> return -1;
> }
> JobConf conf = new JobConf(getConf(), TestNewReducer.class);
> conf.setJobName("test-newreducer");
> conf.setInputFormat(TextInputFormat.class);
> conf.setMapperClass(Map.class);
> conf.setReducerClass(Reduce.class);
> conf.setOutputFormat(TextOutputFormat.class);
> conf.setOutputKeyClass(LongWritable.class);
> conf.setOutputValueClass(Text.class);
> FileInputFormat.setInputPaths(conf, new Path(args[0]));
> FileOutputFormat.setOutputPath(conf, new Path(args[1]));
> JobClient.runJob(conf);
> return 0;
> }
> public static void main(String[] args) throws Exception {
> int res = ToolRunner.run(new Configuration(), new TestNewReducer(),
> args);
> System.exit(res);
> }
> public static class Map extends MapReduceBase implements Mapper
> <LongWritable, Text, LongWritable, Text> {
> private final static LongWritable nill = new LongWritable(1);
> public void map (LongWritable key, Text value,
> OutputCollector<LongWritable, Text> output,
> Reporter reporter) throws IOException {
> output.collect(nill, value);
> }
> }
> public static class Reduce extends MapReduceBase implements
> Reducer<LongWritable, Text, LongWritable, Text> {
> private final static LongWritable nill = new LongWritable(1);
> public void reduce(LongWritable key, Iterator<Text> values,
> OutputCollector<LongWritable, Text> output,
> Reporter reporter) throws IOException {
> Text t1 = null;
> if (values.hasNext())
> t1 = values.next(); // t1 is the first element in values
> while(values.hasNext()) {
> // is this element equal to t1?
> Text t = values.next();
> output.collect(nill, t.equals(t1) ? new Text("Equal") : new
> Text("Not equal"));
> }
> }
> }
> }
> {noformat}
> h2. Possible fixes
> # Return a new object each time for next(). This might have significant
> overhead.
> # Return a new object for unknown object types and reuse the same object for
> known types (like Text). Remove "if (this==anObject) return true;" check from
> all equals() methods for known objects.
> # Document clearly that all user-defined classes must implement an equals()
> method, which doesn't do the "if (this==anObject) return true;" check (ie.
> push the problem to the user).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
