Uploaded image for project: 'Accumulo'
  1. Accumulo
  2. ACCUMULO-2544

Incorrect boundry matching for MockTableOperations.deleteRows

Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 1.5.0, 1.5.1
    • 1.5.2, 1.6.0
    • test
    • Mac OS Mavericks; java 7

    Description

      The api for deleteRows specifies: Delete rows between (start, end] but the current implementation for MockTableOperations.deleteRows is implemented as (start, end)

      Here is the failing test case

      public class TestDelete {
        private static final String INSTANCE = "mock";
        private static final String TABLE = "foo";
        private static final String USER = "user";
        private static final String PASS = "password";
        private static final Authorizations AUTHS = new Authorizations();
      
        @Test
        public void testDelete() throws TableNotFoundException, AccumuloException,
            AccumuloSecurityException, TableExistsException {
      
          MockInstance mockAcc = new MockInstance(INSTANCE);
          Connector conn = mockAcc.getConnector(USER, new PasswordToken(PASS));
          conn.tableOperations().create(TABLE);
          conn.securityOperations().grantTablePermission(USER, TABLE, TablePermission.READ);
          conn.securityOperations().grantTablePermission(USER, TABLE, TablePermission.WRITE);
      
          Mutation mut = new Mutation("2");
          mut.put("colfam", "colqual", "value");
          BatchWriter writer = conn.createBatchWriter(TABLE, new BatchWriterConfig());
          writer.addMutation(mut);
      
          Scanner scan = conn.createScanner(TABLE, AUTHS);
          scan.setRange(new Range("2", "2"));
      
          assertEquals(1, countRecords(scan));
          
          // this should delete (1,2] 
          conn.tableOperations().deleteRows(TABLE, new Text("1"), new Text("2"));
      
          scan = conn.createScanner(TABLE, AUTHS);
          scan.setRange(new Range("2", "2"));
          
          // this will fail if row 2 exists
          assertEquals(0, countRecords(scan));
        }
      
        private int countRecords(Scanner scan) {
          int cnt = 0;
          for (Entry<Key, Value> entry : scan) {
            cnt++;
          }
          scan.close();
          return cnt;
        }
      }
      

      Attachments

        1. ACCUMULO-2544.patch
          2 kB
          Mike Fagan
        2. ACCUMULO-2544.patch
          2 kB
          Mike Fagan
        3. ACCUMULO-2544_v2.patch
          3 kB
          Mike Fagan
        4. ACCUMULO-2544_1.5.2.patch
          3 kB
          Mike Fagan

        Activity

          faganm Mike Fagan added a comment -

          Here is a patch to fix this issue:

          diff --git a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
          index cc6bce7..047dcc4 100644
          — a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
          +++ b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
          @@ -372,7 +372,13 @@ public class MockTableOperations extends TableOperationsHelper {
          if (!exists(tableName))
          throw new TableNotFoundException(tableName, tableName, "");
          MockTable t = acu.tables.get(tableName);

          • Set<Key> keep = new TreeSet<Key>(t.table.tailMap(new Key(start)).headMap(new Key(end)).keySet());
            + byte[] zero = { 0 }

            ;
            + Text startText = new Text(start);
            + Text endText = new Text(end);
            + startText.append( zero, 0, 1);
            + endText.append( zero, 0, 1);
            +
            + Set<Key> keep = new TreeSet<Key>(t.table.subMap(new Key(startText), new Key(endText)).keySet());
            t.table.keySet().removeAll(keep);
            }

          diff --git a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java
          index 216b3ba..42ae119 100644
          — a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java
          +++ b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java
          @@ -268,7 +268,7 @@ public class MockTableOperationsTest {
          to.deleteRows("test", new Text("1"), new Text("2"));
          Scanner s = connector.createScanner("test", Authorizations.EMPTY);
          for (Entry<Key,Value> entry : s)

          { - Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '1'); + Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '2'); }

          }

          faganm Mike Fagan added a comment - Here is a patch to fix this issue: diff --git a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java index cc6bce7..047dcc4 100644 — a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java +++ b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java @@ -372,7 +372,13 @@ public class MockTableOperations extends TableOperationsHelper { if (!exists(tableName)) throw new TableNotFoundException(tableName, tableName, ""); MockTable t = acu.tables.get(tableName); Set<Key> keep = new TreeSet<Key>(t.table.tailMap(new Key(start)).headMap(new Key(end)).keySet()); + byte[] zero = { 0 } ; + Text startText = new Text(start); + Text endText = new Text(end); + startText.append( zero, 0, 1); + endText.append( zero, 0, 1); + + Set<Key> keep = new TreeSet<Key>(t.table.subMap(new Key(startText), new Key(endText)).keySet()); t.table.keySet().removeAll(keep); } diff --git a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java index 216b3ba..42ae119 100644 — a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java +++ b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java @@ -268,7 +268,7 @@ public class MockTableOperationsTest { to.deleteRows("test", new Text("1"), new Text("2")); Scanner s = connector.createScanner("test", Authorizations.EMPTY); for (Entry<Key,Value> entry : s) { - Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '1'); + Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '2'); } }
          elserj Josh Elser added a comment -

          faganm, please attach a patch following the instructions here. This helps us both apply your changes and properly give you credit for the work you have contributed. Thanks!

          elserj Josh Elser added a comment - faganm , please attach a patch following the instructions here . This helps us both apply your changes and properly give you credit for the work you have contributed. Thanks!
          faganm Mike Fagan added a comment -

          From d35862f4dae9da6746b6195da73e21a01d6b324a Mon Sep 17 00:00:00 2001
          From: Michael Fagan <mfagan@arcus-research.com>
          Date: Mon, 24 Mar 2014 20:27:57 -0600
          Subject: [PATCH] ACCUMULO-2544 - fix match boundaries for
          MockTableOperations.deleteRows to be consistent with actual accumulo instance

          —
          .../org/apache/accumulo/core/client/mock/MockTableOperations.java | 8 +++++++-
          .../apache/accumulo/core/client/mock/MockTableOperationsTest.java | 2 +-
          2 files changed, 8 insertions, 2 deletions

          diff --git a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
          index cc6bce7..047dcc4 100644
          — a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
          +++ b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
          @@ -372,7 +372,13 @@ public class MockTableOperations extends TableOperationsHelper {
          if (!exists(tableName))
          throw new TableNotFoundException(tableName, tableName, "");
          MockTable t = acu.tables.get(tableName);

          • Set<Key> keep = new TreeSet<Key>(t.table.tailMap(new Key(start)).headMap(new Key(end)).keySet());
            + byte[] zero = { 0 }

            ;
            + Text startText = new Text(start);
            + Text endText = new Text(end);
            + startText.append( zero, 0, 1);
            + endText.append( zero, 0, 1);
            +
            + Set<Key> keep = new TreeSet<Key>(t.table.subMap(new Key(startText), new Key(endText)).keySet());
            t.table.keySet().removeAll(keep);
            }

          diff --git a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java
          index 216b3ba..42ae119 100644
          — a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java
          +++ b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java
          @@ -268,7 +268,7 @@ public class MockTableOperationsTest {
          to.deleteRows("test", new Text("1"), new Text("2"));
          Scanner s = connector.createScanner("test", Authorizations.EMPTY);
          for (Entry<Key,Value> entry : s)

          { - Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '1'); + Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '2'); }

          }

          –
          1.8.5.2 (Apple Git-48)

          faganm Mike Fagan added a comment - From d35862f4dae9da6746b6195da73e21a01d6b324a Mon Sep 17 00:00:00 2001 From: Michael Fagan <mfagan@arcus-research.com> Date: Mon, 24 Mar 2014 20:27:57 -0600 Subject: [PATCH] ACCUMULO-2544 - fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance — .../org/apache/accumulo/core/client/mock/MockTableOperations.java | 8 +++++++- .../apache/accumulo/core/client/mock/MockTableOperationsTest.java | 2 +- 2 files changed, 8 insertions , 2 deletions diff --git a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java index cc6bce7..047dcc4 100644 — a/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java +++ b/core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java @@ -372,7 +372,13 @@ public class MockTableOperations extends TableOperationsHelper { if (!exists(tableName)) throw new TableNotFoundException(tableName, tableName, ""); MockTable t = acu.tables.get(tableName); Set<Key> keep = new TreeSet<Key>(t.table.tailMap(new Key(start)).headMap(new Key(end)).keySet()); + byte[] zero = { 0 } ; + Text startText = new Text(start); + Text endText = new Text(end); + startText.append( zero, 0, 1); + endText.append( zero, 0, 1); + + Set<Key> keep = new TreeSet<Key>(t.table.subMap(new Key(startText), new Key(endText)).keySet()); t.table.keySet().removeAll(keep); } diff --git a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java index 216b3ba..42ae119 100644 — a/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java +++ b/core/src/test/java/org/apache/accumulo/core/client/mock/MockTableOperationsTest.java @@ -268,7 +268,7 @@ public class MockTableOperationsTest { to.deleteRows("test", new Text("1"), new Text("2")); Scanner s = connector.createScanner("test", Authorizations.EMPTY); for (Entry<Key,Value> entry : s) { - Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '1'); + Assert.assertTrue(entry.getKey().getRow().toString().charAt(0) != '2'); } } – 1.8.5.2 (Apple Git-48)
          faganm Mike Fagan added a comment -

          This seams like a really odd way to submit a patch...
          I would have though I should use git send-email

          faganm Mike Fagan added a comment - This seams like a really odd way to submit a patch... I would have though I should use git send-email
          kturner Keith Turner added a comment -

          You can attach patch files to a ticket. Go to More->Attach Files.

          kturner Keith Turner added a comment - You can attach patch files to a ticket. Go to More->Attach Files.
          mdrob Mike Drob added a comment -

          faganm - you might have been missing the appropriate JIRA permissions to attach files to issues. I have added you to our contributors group and assigned the issue to you. Please let me know how you would like to appear on our Contributors page. (http://accumulo.apache.org/people.html)

          mdrob Mike Drob added a comment - faganm - you might have been missing the appropriate JIRA permissions to attach files to issues. I have added you to our contributors group and assigned the issue to you. Please let me know how you would like to appear on our Contributors page. ( http://accumulo.apache.org/people.html )
          faganm Mike Fagan added a comment -

          Patch is attached

          faganm Mike Fagan added a comment - Patch is attached
          faganm Mike Fagan added a comment - - edited

          Mike Drob,

          Can you please add the following contributor row once the patch is accepted:

          <tr><td>faganm</td><td>Mike Fagan</td><td><a href="http://www.arcus-research.com">Arcus Research</a></td><td>MT (<a href="http://www.timeanddate.com/library/abbreviations/timezones/na/mst.html">-7</a> / <a href="http://www.timeanddate.com/library/abbreviations/timezones/na/mdt.html">-6</a>)</td></tr>

          faganm Mike Fagan added a comment - - edited Mike Drob, Can you please add the following contributor row once the patch is accepted: <tr><td>faganm</td><td>Mike Fagan</td><td><a href="http://www.arcus-research.com">Arcus Research</a></td><td>MT (<a href="http://www.timeanddate.com/library/abbreviations/timezones/na/mst.html">-7</a> / <a href="http://www.timeanddate.com/library/abbreviations/timezones/na/mdt.html">-6</a>)</td></tr>
          busbey Sean Busbey added a comment -

          faganm, could you move

              byte[] zero = { 0 };
          

          Into a private static final member?

          Also, it looks like this same incorrect behavior is in 1.5.x. Could you make sure your patch applies against the 1.5.2-SNAPSHOT branch? A committer will take care of merging the fix forward into 1.6 and master.

          busbey Sean Busbey added a comment - faganm , could you move byte[] zero = { 0 }; Into a private static final member? Also, it looks like this same incorrect behavior is in 1.5.x. Could you make sure your patch applies against the 1.5.2-SNAPSHOT branch? A committer will take care of merging the fix forward into 1.6 and master.
          faganm Mike Fagan added a comment -

          Sean,

          Sure I will move the var and create a new patch against 1.5.2-SNAPSHOT

          faganm Mike Fagan added a comment - Sean, Sure I will move the var and create a new patch against 1.5.2-SNAPSHOT
          faganm Mike Fagan added a comment -

          Attached patch for 1.5.2-SNAPSHOT

          faganm Mike Fagan added a comment - Attached patch for 1.5.2-SNAPSHOT
          busbey Sean Busbey added a comment -

          Thanks Mike!

          Sorry I missed this the first time, but could you change the correctness assertion at the end of the test to also make sure row 1 is not gone? Since the previous implementation was also wrong about the lower bound, I'd like to make sure we check it.

          busbey Sean Busbey added a comment - Thanks Mike! Sorry I missed this the first time, but could you change the correctness assertion at the end of the test to also make sure row 1 is not gone? Since the previous implementation was also wrong about the lower bound, I'd like to make sure we check it.
          faganm Mike Fagan added a comment -

          Patch for 1.5.2-SNAPSHOT with boundary checking.

          faganm Mike Fagan added a comment - Patch for 1.5.2-SNAPSHOT with boundary checking.

          Commit 7ec60f1bcac4c274653d2779f1996138f3a275b7 in accumulo's branch refs/heads/1.5.2-SNAPSHOT from mikebfagan
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=7ec60f1 ]

          ACCUMULO-2544: Fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance

          Signed-off-by: Sean Busbey <busbey@cloudera.com>

          jira-bot ASF subversion and git services added a comment - Commit 7ec60f1bcac4c274653d2779f1996138f3a275b7 in accumulo's branch refs/heads/1.5.2-SNAPSHOT from mikebfagan [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=7ec60f1 ] ACCUMULO-2544 : Fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance Signed-off-by: Sean Busbey <busbey@cloudera.com>

          Commit 7ec60f1bcac4c274653d2779f1996138f3a275b7 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from mikebfagan
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=7ec60f1 ]

          ACCUMULO-2544: Fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance

          Signed-off-by: Sean Busbey <busbey@cloudera.com>

          jira-bot ASF subversion and git services added a comment - Commit 7ec60f1bcac4c274653d2779f1996138f3a275b7 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from mikebfagan [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=7ec60f1 ] ACCUMULO-2544 : Fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance Signed-off-by: Sean Busbey <busbey@cloudera.com>

          Commit 7ec60f1bcac4c274653d2779f1996138f3a275b7 in accumulo's branch refs/heads/master from mikebfagan
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=7ec60f1 ]

          ACCUMULO-2544: Fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance

          Signed-off-by: Sean Busbey <busbey@cloudera.com>

          jira-bot ASF subversion and git services added a comment - Commit 7ec60f1bcac4c274653d2779f1996138f3a275b7 in accumulo's branch refs/heads/master from mikebfagan [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=7ec60f1 ] ACCUMULO-2544 : Fix match boundaries for MockTableOperations.deleteRows to be consistent with actual accumulo instance Signed-off-by: Sean Busbey <busbey@cloudera.com>

          Commit 1582400 from busbey@apache.org in branch 'site/trunk'
          [ https://svn.apache.org/r1582400 ]

          updated contributor list for ACCUMULO-1945 and ACCUMULO-2544

          jira-bot ASF subversion and git services added a comment - Commit 1582400 from busbey@apache.org in branch 'site/trunk' [ https://svn.apache.org/r1582400 ] updated contributor list for ACCUMULO-1945 and ACCUMULO-2544

          People

            faganm Mike Fagan
            faganm Mike Fagan
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: