From bf84fc9cf1480103f20b0dc70363148e799df46d Mon Sep 17 00:00:00 2001 From: Lu Liu <2w6f8c@gmail.com> Date: Mon, 20 Apr 2020 15:07:09 +0800 Subject: [PATCH] use string representation for matching (#2571) --- .../com/google/refine/LookupCacheManager.java | 11 ++-- .../google/refine/expr/functions/Cross.java | 6 +- .../refine/expr/functions/CrossTests.java | 66 ++++++++++++++++++- 3 files changed, 73 insertions(+), 10 deletions(-) diff --git a/main/src/com/google/refine/LookupCacheManager.java b/main/src/com/google/refine/LookupCacheManager.java index 7a1faa20d..91cc2e5c9 100644 --- a/main/src/com/google/refine/LookupCacheManager.java +++ b/main/src/com/google/refine/LookupCacheManager.java @@ -87,8 +87,9 @@ public class LookupCacheManager { Row targetRow = targetProject.rows.get(r); Object value = targetRow.getCellValue(targetColumn.getCellIndex()); if (ExpressionUtils.isNonBlankData(value)) { - lookup.valueToRowIndices.putIfAbsent(value, new ArrayList<>()); - lookup.valueToRowIndices.get(value).add(r); + String valueStr = value.toString(); + lookup.valueToRowIndices.putIfAbsent(valueStr, new ArrayList<>()); + lookup.valueToRowIndices.get(valueStr).add(r); } } } @@ -106,11 +107,13 @@ public class LookupCacheManager { } public HasFieldsListImpl getRows(Object value) { - if (ExpressionUtils.isNonBlankData(value) && valueToRowIndices.containsKey(value)) { + if (!ExpressionUtils.isNonBlankData(value)) return null; + String valueStr = value.toString(); + if (valueToRowIndices.containsKey(valueStr)) { Project targetProject = ProjectManager.singleton.getProject(targetProjectID); if (targetProject != null) { HasFieldsListImpl rows = new HasFieldsListImpl(); - for (Integer r : valueToRowIndices.get(value)) { + for (Integer r : valueToRowIndices.get(valueStr)) { Row row = targetProject.rows.get(r); rows.add(new WrappedRow(targetProject, r, row)); } diff --git a/main/src/com/google/refine/expr/functions/Cross.java b/main/src/com/google/refine/expr/functions/Cross.java index 5b74e3d90..30dd398f6 100644 --- a/main/src/com/google/refine/expr/functions/Cross.java +++ b/main/src/com/google/refine/expr/functions/Cross.java @@ -76,17 +76,17 @@ public class Cross implements Function { } } } - return new EvalError(ControlFunctionRegistry.getFunctionName(this) + " expects a cell or cell value, a project name to look up, and a column name in that project"); + return new EvalError(ControlFunctionRegistry.getFunctionName(this) + " expects a cell or value, a project name to look up, and a column name in that project"); } @Override public String getDescription() { - return "Looks up the given value in the target column of the target project, returns an array of matched rows, cell will be interpreted as cell.value"; + return "Looks up the given value in the target column of the target project, returns an array of matched rows, cell will be interpreted as cell.value. Two values match if and only if they have the same string representation"; } @Override public String getParams() { - return "cell c or object value, string projectName, string columnName"; + return "cell or value, string projectName, string columnName"; } @Override diff --git a/main/tests/server/src/com/google/refine/expr/functions/CrossTests.java b/main/tests/server/src/com/google/refine/expr/functions/CrossTests.java index edadf1a7b..a88067a61 100644 --- a/main/tests/server/src/com/google/refine/expr/functions/CrossTests.java +++ b/main/tests/server/src/com/google/refine/expr/functions/CrossTests.java @@ -149,8 +149,10 @@ public class CrossTests extends RefineTest { Assert.assertEquals(address, "50 Broadway Ave."); } + /** + * The result shouldn't depend on the based column in "bindings" when the first argument is a WrappedCell instance. + */ @Test - // cross result shouldn't depend on the based column in "bindings" when the first argument is a WrappedCell instance public void crossFunctionDifferentColumnTest() throws Exception { Project project = (Project) bindings.get("project"); bindings.put("columnName", "gift"); // change the based column @@ -221,6 +223,29 @@ public class CrossTests extends RefineTest { Assert.assertEquals(address, "integer"); } + @Test + public void crossFunctionIntegerArgumentTest1() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", 1600L, "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "integer"); + } + + @Test + public void crossFunctionIntegerArgumentTest2() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", "1600", "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "integer"); + } + + /** + * Two values will match if and only if they have the same string representation. + * In this case, "1600.0" doesn't equal to "1600". + */ + @Test + public void crossFunctionIntegerArgumentTest3() throws Exception { + Assert.assertNull(invoke("cross", "1600.0", "My Address Book", "friend")); + } + @Test public void crossFunctionLongArgumentTest() throws Exception { Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", 123456789123456789L, "My Address Book", "friend")).get(0)).row); @@ -228,6 +253,13 @@ public class CrossTests extends RefineTest { Assert.assertEquals(address, "long"); } + @Test + public void crossFunctionLongArgumentTest1() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", "123456789123456789", "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "long"); + } + @Test public void crossFunctionDoubleArgumentTest() throws Exception { Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", 3.14, "My Address Book", "friend")).get(0)).row); @@ -235,6 +267,20 @@ public class CrossTests extends RefineTest { Assert.assertEquals(address, "double"); } + @Test + public void crossFunctionDoubleArgumentTest1() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", 3.14f, "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "double"); + } + + @Test + public void crossFunctionDoubleArgumentTest2() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", "3.14", "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "double"); + } + @Test public void crossFunctionDateTimeArgumentTest() throws Exception { Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", dateTimeValue, "My Address Book", "friend")).get(0)).row); @@ -242,6 +288,13 @@ public class CrossTests extends RefineTest { Assert.assertEquals(address, "dateTime"); } + @Test + public void crossFunctionDateTimeArgumentTest1() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", dateTimeValue.toString(), "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "dateTime"); + } + @Test public void crossFunctionBooleanArgumentTest() throws Exception { Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", true, "My Address Book", "friend")).get(0)).row); @@ -249,6 +302,13 @@ public class CrossTests extends RefineTest { Assert.assertEquals(address, "boolean"); } + @Test + public void crossFunctionBooleanArgumentTest1() throws Exception { + Row row = (((WrappedRow) ((HasFieldsListImpl) invoke("cross", "true", "My Address Book", "friend")).get(0)).row); + String address = row.getCell(1).value.toString(); + Assert.assertEquals(address, "boolean"); + } + /** * If no match, return null. * @@ -268,7 +328,7 @@ public class CrossTests extends RefineTest { @Test public void crossFunctionNonLiteralValue() throws Exception { Assert.assertEquals(((EvalError) invoke("cross", null, "My Address Book", "friend")).message, - "cross expects a cell or cell value, a project name to look up, and a column name in that project"); + "cross expects a cell or value, a project name to look up, and a column name in that project"); } /** @@ -289,7 +349,7 @@ public class CrossTests extends RefineTest { @Test public void serializeCross() { - String json = "{\"description\":\"Looks up the given value in the target column of the target project, returns an array of matched rows, cell will be interpreted as cell.value\",\"params\":\"cell c or object value, string projectName, string columnName\",\"returns\":\"array\"}"; + String json = "{\"description\":\"Looks up the given value in the target column of the target project, returns an array of matched rows, cell will be interpreted as cell.value. Two values match if and only if they have the same string representation\",\"params\":\"cell or value, string projectName, string columnName\",\"returns\":\"array\"}"; TestUtils.isSerializedTo(new Cross(), json); } }