hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Spyros Blanas (JIRA)" <j...@apache.org>
Subject [jira] Created: (HADOOP-3522) ValuesIterator.next() doesn't return a new object, thus failing many equals() tests.
Date Tue, 10 Jun 2008 02:01:45 GMT
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


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.


Mime
View raw message