Repository: jtablesaw/tablesaw
Pull request #604 (2019-08-17 05:03:05)

Move the Sort prefix logic from Table class to Sort class.

- [x] Tick to sign-off your agreement to the [Developer Certificate of Origin (DCO) 1.1](https://developercertificate.org)

## Description

* Move Sort prefix logic (+, -) from Table to Sort, so it can be used elsewhere.
* Improve  error messages when the prefix is invalid or the column does not exist on the table.

## Testing

Yes. Improved testing of error messages.

ryancerf: The new method on Sort is a static constructor. It creates a sort object, but does not sort the table.

The method is very very poorly named on my part. How about renaming it to `create(table, "col1", "col2")`

Patch:
diff --git a/core/src/main/java/tech/tablesaw/api/Table.java b/core/src/main/java/tech/tablesaw/api/Table.java
index 45885b8a..fff17d94 100644
--- a/core/src/main/java/tech/tablesaw/api/Table.java
+++ b/core/src/main/java/tech/tablesaw/api/Table.java
@@ -530,45 +530,7 @@ public class Table extends Relation implements Iterable<Row> {
    * <p>if column name starts with - then sort that column descending otherwise sort ascending
    */
   public Table sortOn(String... columnNames) {
-
-    Sort key = null;
-    List<String> names = columnNames().stream().map(String::toUpperCase).collect(toList());
-
-    for (String columnName : columnNames) {
-      Sort.Order order = Sort.Order.ASCEND;
-      if (!names.contains(columnName.toUpperCase())) {
-        // the column name has been annotated with a prefix.
-        // get the prefix which could be - or +
-        String prefix = columnName.substring(0, 1);
-
-        // remove - prefix so provided name matches actual column name
-        columnName = columnName.substring(1, columnName.length());
-
-        order = getOrder(prefix);
-      }
-
-      if (key == null) { // key will be null the first time through
-        key = first(columnName, order);
-      } else {
-        key.next(columnName, order);
-      }
-    }
-    return sortOn(key);
-  }
-
-  private Sort.Order getOrder(String prefix) {
-    Sort.Order order;
-    switch (prefix) {
-      case "+":
-        order = Sort.Order.ASCEND;
-        break;
-      case "-":
-        order = Sort.Order.DESCEND;
-        break;
-      default:
-        throw new IllegalStateException("Column prefix: " + prefix + " is unknown.");
-    }
-    return order;
+    return this.sortOn(Sort.create(this, columnNames));
   }
 
   /**
diff --git a/core/src/main/java/tech/tablesaw/columns/AbstractColumn.java b/core/src/main/java/tech/tablesaw/columns/AbstractColumn.java
index f2656ed3..ecc2e543 100644
--- a/core/src/main/java/tech/tablesaw/columns/AbstractColumn.java
+++ b/core/src/main/java/tech/tablesaw/columns/AbstractColumn.java
@@ -58,8 +58,8 @@ public abstract class AbstractColumn<T> implements Column<T> {
   @Override
   public StringColumn asStringColumn() {
     StringColumn sc = StringColumn.create(name() + " strings");
-    for (T value : this) {
-      sc.append(String.valueOf(value));
+    for(int i = 0; i < size(); i++) {
+    	sc.append(getUnformattedString(i));
     }
     return sc;
   }
diff --git a/core/src/main/java/tech/tablesaw/columns/Column.java b/core/src/main/java/tech/tablesaw/columns/Column.java
index 5dc97534..21780391 100644
--- a/core/src/main/java/tech/tablesaw/columns/Column.java
+++ b/core/src/main/java/tech/tablesaw/columns/Column.java
@@ -617,5 +617,10 @@ public interface Column<T> extends Iterable<T>, Comparator<T> {
     return where(selectNRowsAtRandom(tableSize, size()));
   }
 
+  /**
+   * Returns a StringColumn consisting of the (unformatted) String representation of this column values
+   * @return a {@link StringColumn} built using the column {@link #getUnformattedString} method
+   */
   StringColumn asStringColumn();
+
 }
diff --git a/core/src/main/java/tech/tablesaw/sorting/Sort.java b/core/src/main/java/tech/tablesaw/sorting/Sort.java
index fffb5c98..ddcbc563 100644
--- a/core/src/main/java/tech/tablesaw/sorting/Sort.java
+++ b/core/src/main/java/tech/tablesaw/sorting/Sort.java
@@ -14,10 +14,16 @@
 
 package tech.tablesaw.sorting;
 
+import static java.util.stream.Collectors.toSet;
+
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import tech.tablesaw.api.Table;
 
 /**
  * Provides fine-grained control over sorting.
@@ -56,6 +62,71 @@ public class Sort implements Iterable<Map.Entry<String, Sort.Order>> {
     return sortOrder.size();
   }
 
+  /**
+   * Create a Sort object from the given table and sort column names. Does not sort the table.
+   *
+   * @param table to sort. Used only to pull the table's schema. Does not modify the table.
+   * @param columnNames The columns to sort on. Can prefix column name with + for ascending, - for descending.
+   * Default to ascending if no prefix is added.
+   * @return a {@link #Sort} Object.
+   */
+  public static Sort create(Table table, String... columnNames) {
+    Preconditions.checkArgument(columnNames.length > 0,
+      "At least one sort column must provided.");
+
+    Sort key = null;
+    Set<String> names = table.columnNames().stream().map(String::toUpperCase).collect(toSet());
+
+    for (String columnName : columnNames) {
+      Sort.Order order = Sort.Order.ASCEND;
+      if (!names.contains(columnName.toUpperCase())) {
+        // the column name has been annotated with a prefix.
+        // get the prefix which could be - or +
+        String prefix = columnName.substring(0, 1);
+        Optional<Order> orderOptional = getOrder(prefix);
+
+        // Invalid prefix, column name exists on table.
+        if(!orderOptional.isPresent() && names.contains(columnName.substring(1).toUpperCase())) {
+          throw new IllegalStateException("Column prefix: " + prefix + " is unknown.");
+        }
+
+        // Valid prefix, column name does not exist on table.
+        if(orderOptional.isPresent() && !names.contains(columnName.substring(1).toUpperCase())) {
+          throw new IllegalStateException(
+            String.format("Column %s does not exist in table %s", columnName.substring(1), table.name()));
+        }
+
+        // Invalid prefix, column name does not exist on table.
+        if(!orderOptional.isPresent()) {
+          throw new IllegalStateException("Unrecognized Column: '" + columnName + "'");
+        }
+
+        // Valid Prefix, column name exists on table.
+        // remove - prefix so provided name matches actual column name
+        columnName = columnName.substring(1);
+        order = orderOptional.get();
+      }
+
+      if (key == null) { // key will be null the first time through
+        key = new Sort(columnName, order);
+      } else {
+        key.next(columnName, order);
+      }
+    }
+    return key;
+  }
+
+  private static Optional<Order> getOrder(String prefix) {
+    switch (prefix) {
+      case "+":
+        return Optional.of(Order.ASCEND);
+      case "-":
+        return Optional.of(Order.DESCEND);
+      default:
+        return Optional.empty();
+    }
+  }
+
   /**
    * Returns an iterator over elements of type {@code T}.
    *


Test patch:
diff --git a/core/src/test/java/tech/tablesaw/SortTest.java b/core/src/test/java/tech/tablesaw/SortTest.java
index 48b6079f..185117de 100644
--- a/core/src/test/java/tech/tablesaw/SortTest.java
+++ b/core/src/test/java/tech/tablesaw/SortTest.java
@@ -15,11 +15,14 @@
 package tech.tablesaw;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
+import tech.tablesaw.api.DoubleColumn;
 import tech.tablesaw.api.Table;
+import tech.tablesaw.sorting.Sort;
 
 /** Verify sorting functions */
 public class SortTest {
@@ -122,6 +125,33 @@ public class SortTest {
     assertTablesEquals(expectedResults, sortedTable);
   }
 
+  @Test
+  public void createSortInvalidPrefixColumnExists() {
+    Table table = Table.create("t", DoubleColumn.create("col1"));
+    Throwable thrown = assertThrows(IllegalStateException.class, () ->
+      Sort.create(table, "<col1"));
+
+    assertEquals("Column prefix: < is unknown.", thrown.getMessage());
+  }
+
+  @Test
+  public void createSortValidPrefixColumnDoesNotExist() {
+    Table table = Table.create("t", DoubleColumn.create("col1"));
+    Throwable thrown = assertThrows(IllegalStateException.class, () ->
+      Sort.create(table, "+col2"));
+
+    assertEquals("Column col2 does not exist in table t", thrown.getMessage());
+  }
+
+  @Test
+  public void createSortInvalidPrefixColumnDoesNotExist() {
+    Table table = Table.create("t", DoubleColumn.create("col1"));
+    Throwable thrown = assertThrows(IllegalStateException.class, () ->
+      Sort.create(table, ">col2"));
+
+    assertEquals("Unrecognized Column: '>col2'", thrown.getMessage());
+  }
+
   /**
    * Make sure each row in each table match
    *
diff --git a/core/src/test/java/tech/tablesaw/api/DateTimeColumnTest.java b/core/src/test/java/tech/tablesaw/api/DateTimeColumnTest.java
index fc8b56e7..fdde6a14 100644
--- a/core/src/test/java/tech/tablesaw/api/DateTimeColumnTest.java
+++ b/core/src/test/java/tech/tablesaw/api/DateTimeColumnTest.java
@@ -24,6 +24,8 @@ import java.time.format.DateTimeFormatter;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import tech.tablesaw.columns.strings.StringColumnType;
+
 public class DateTimeColumnTest {
 
   private DateTimeColumn column1;
@@ -109,4 +111,15 @@ public class DateTimeColumnTest {
 	  assertEquals("2000-01-01T00:00:00", column1.getString(0));
 	  assertEquals("NaT", column1.getString(1));
   }
+
+  @Test
+  public void testAsStringColumn() {
+    column1.appendCell("1923-10-20T10:15:30");
+    column1.appendMissing();
+    StringColumn sc = column1.asStringColumn();
+    assertEquals("Game date strings", sc.name());
+    assertEquals(2, sc.size());
+    assertEquals("1923-10-20T10:15:30.000", sc.get(0));
+    assertEquals(StringColumnType.missingValueIndicator(), sc.get(1));
+  }
 }
diff --git a/core/src/test/java/tech/tablesaw/api/NumberColumnTest.java b/core/src/test/java/tech/tablesaw/api/NumberColumnTest.java
index 599bf8aa..b540613d 100644
--- a/core/src/test/java/tech/tablesaw/api/NumberColumnTest.java
+++ b/core/src/test/java/tech/tablesaw/api/NumberColumnTest.java
@@ -48,6 +48,7 @@ import org.junit.jupiter.api.Test;
 import tech.tablesaw.columns.Column;
 import tech.tablesaw.columns.numbers.DoubleColumnType;
 import tech.tablesaw.columns.numbers.NumberColumnFormatter;
+import tech.tablesaw.columns.strings.StringColumnType;
 import tech.tablesaw.selection.Selection;
 
 /** Unit tests for the NumberColumn class */
@@ -319,6 +320,28 @@ public class NumberColumnTest {
     assertEquals("48392.2932", sc.get(0));
   }
 
+  @Test
+  public void testDoubleColumnAsStringWithMissing() {
+    DoubleColumn numberColumn = DoubleColumn.create("test");
+    numberColumn.append(48392.2932);
+    numberColumn.appendMissing();
+    StringColumn sc = numberColumn.asStringColumn();
+    assertEquals("test strings", sc.name());
+    assertEquals("48392.2932", sc.get(0));
+    assertEquals(StringColumnType.missingValueIndicator(), sc.get(1));
+  }
+
+  @Test
+  public void testIntegerColumnAsStringWithMissing() {
+    IntColumn numberColumn = IntColumn.create("test");
+    numberColumn.append(48392);
+    numberColumn.appendMissing();
+    StringColumn sc = numberColumn.asStringColumn();
+    assertEquals("test strings", sc.name());
+    assertEquals("48392", sc.get(0));
+    assertEquals(StringColumnType.missingValueIndicator(), sc.get(1));
+  }
+
   @Test
   public void testNumberFormat3() {
     DoubleColumn numberColumn = DoubleColumn.create("test");