From d57d76f7dfd875a79a557b84c0ca3a12ce6f544d Mon Sep 17 00:00:00 2001
From: Joanne Ong <joanneongcf@u.nus.edu>
Date: Tue, 16 Jun 2020 01:38:50 +0800
Subject: [PATCH] Fix imprecise facet statistics in records mode (#2607)

* Fix bug in choice counts for records mode

* Add test for value grouper on records

* Refactor and comment code

* Count distinct instances of null/blank data

* Update test to check for blank data count in records

* Remove unnecessary import statement
---
 .../util/ExpressionNominalValueGrouper.java   | 18 +++++------
 .../ExpressionNominalValueGrouperTests.java   | 32 ++++++++++++++++++-
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/main/src/com/google/refine/browsing/util/ExpressionNominalValueGrouper.java b/main/src/com/google/refine/browsing/util/ExpressionNominalValueGrouper.java
index a0039d7ce..a2bc1c828 100644
--- a/main/src/com/google/refine/browsing/util/ExpressionNominalValueGrouper.java
+++ b/main/src/com/google/refine/browsing/util/ExpressionNominalValueGrouper.java
@@ -123,21 +123,21 @@ public class ExpressionNominalValueGrouper implements RowVisitor, RecordVisitor
 
     @Override
     public boolean visit(Project project, Record record) {
-        hasError = false;
-        hasBlank = false;
-
         Properties bindings = ExpressionUtils.createBindings(project);
 
         for (int r = record.fromRowIndex; r < record.toRowIndex; r++) {
+            hasError = false;
+            hasBlank = false;
+
             Row row = project.rows.get(r);
             visitRow(project, r, row, bindings, record.recordIndex);
-        }
 
-        if (hasError) {
-            errorCount++;
-        }
-        if (hasBlank) {
-            blankCount++;
+            if (hasError) {
+                errorCount++;
+            }
+            if (hasBlank) {
+                blankCount++;
+            }
         }
 
         return false;
diff --git a/main/tests/server/src/com/google/refine/browsing/util/ExpressionNominalValueGrouperTests.java b/main/tests/server/src/com/google/refine/browsing/util/ExpressionNominalValueGrouperTests.java
index b9d37bef8..00ec32a6c 100644
--- a/main/tests/server/src/com/google/refine/browsing/util/ExpressionNominalValueGrouperTests.java
+++ b/main/tests/server/src/com/google/refine/browsing/util/ExpressionNominalValueGrouperTests.java
@@ -51,7 +51,6 @@ import com.google.refine.model.ModelException;
 import com.google.refine.model.Project;
 import com.google.refine.model.Row;
 
-
 public class ExpressionNominalValueGrouperTests extends RefineTest {
     // dependencies    
     //Variables
@@ -174,4 +173,35 @@ public class ExpressionNominalValueGrouperTests extends RefineTest {
       Assert.assertEquals(grouper.choices.get(dateTimeStringValue).decoratedValue.label,dateTimeStringValue);
       Assert.assertEquals(grouper.choices.get(dateTimeStringValue).decoratedValue.value.toString(),dateTimeStringValue);
     }
+
+    @Test
+    public void expressionNominalValueGrouperRecords() throws Exception {
+        String completeProjectJson = "col1,col2,col3\n"
+                + "record1,1,a\n"
+                + ",,a\n"
+                + ",,a\n"
+                + "record2,,a\n"
+                + ",1,a\n";
+
+        project = createCSVProject(completeProjectJson);
+        bindings = new Properties();
+        bindings.put("project", project);
+
+        eval = MetaParser.parse("value");
+        grouper = new ExpressionNominalValueGrouper(eval, "col2", 1);
+        try {
+            grouper.start(project);
+            int c = project.recordModel.getRecordCount();
+            for (int r = 0; r < c; r++) {
+                grouper.visit(project, project.recordModel.getRecord(r));
+            }
+        } finally {
+            grouper.end(project);
+        }
+
+        Assert.assertEquals(grouper.blankCount, 3);
+        Assert.assertEquals(grouper.choices.size(), 1);
+        Assert.assertTrue(grouper.choices.containsKey(integerStringValue));
+        Assert.assertEquals(grouper.choices.get(integerStringValue).count, 2);
+    }
 }