Rework sorting a bit for table tabular reporters (#138)

Author: ryanrdoherty

Model/src/main/java/org/gusdb/wdk/model/answer/AnswerValue.java

@@ -8,7 +8,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -55,14 +54,15 @@
 import org.gusdb.wdk.model.question.Question;
 import org.gusdb.wdk.model.record.Field;
 import org.gusdb.wdk.model.record.PrimaryKeyDefinition;
-import org.gusdb.wdk.model.record.PrimaryKeyValue;
 import org.gusdb.wdk.model.record.PrimaryKeyIterator;
+import org.gusdb.wdk.model.record.PrimaryKeyValue;
 import org.gusdb.wdk.model.record.RecordClass;
 import org.gusdb.wdk.model.record.RecordInstance;
 import org.gusdb.wdk.model.record.ResultSetPrimaryKeyIterator;
 import org.gusdb.wdk.model.record.TableField;
 import org.gusdb.wdk.model.record.attribute.AttributeField;
 import org.gusdb.wdk.model.record.attribute.ColumnAttributeField;
+import org.gusdb.wdk.model.record.attribute.PkColumnAttributeField;
 import org.gusdb.wdk.model.record.attribute.QueryColumnAttributeField;
 import org.gusdb.wdk.model.user.StepContainer;
 import org.gusdb.wdk.model.user.User;
@@ -159,8 +159,8 @@ public class AnswerValue implements PartitionKeysProvider {
   private int _startIndex = 1;
   private int _endIndex = UNBOUNDED_END_PAGE_INDEX;
 
-  // sorting for this answer
-  private Map<String, Boolean> _sortingMap;
+  // sorting for this answer (true = ascending)
+  private LinkedHashMap<String, Boolean> _sortingMap;
 
   // values generated and cached from the above
   private String _sortedIdSql;
@@ -179,7 +179,7 @@ public class AnswerValue implements PartitionKeysProvider {
    * @param avoidCacheHit 
    */
   public AnswerValue(RunnableObj<AnswerSpec> validAnswerSpec, int startIndex,
-      int endIndex, Map<String, Boolean> sortingMap, boolean avoidCacheHit) throws WdkModelException {
+      int endIndex, LinkedHashMap<String, Boolean> sortingMap, boolean avoidCacheHit) throws WdkModelException {
     _validAnswerSpec = validAnswerSpec;
     _answerSpec = validAnswerSpec.get();
     _question = _answerSpec.getQuestion().get(); // must be present if answerspec is valid
@@ -188,7 +188,7 @@ public AnswerValue(RunnableObj<AnswerSpec> validAnswerSpec, int startIndex,
     _avoidCacheHit = avoidCacheHit;
     _idsQueryInstance = Query.makeQueryInstance(_answerSpec.getQueryInstanceSpec().getRunnable().getLeft(), avoidCacheHit, this);
     _resultSizeFactory = new ResultSizeFactory(this);
-    _sortingMap = sortingMap == null? new HashMap<String, Boolean>() : sortingMap;
+    _sortingMap = sortingMap == null? new LinkedHashMap<String, Boolean>() : sortingMap;
     setPageIndex(startIndex, endIndex);
     LOG.debug("AnswerValue created for question: " + _question.getDisplayName());
   }
@@ -508,35 +508,17 @@ public String getFilteredAttributeSql(
     final Query attrQuery,
     final boolean sort
   ) throws WdkModelException {
-    String wrapped = joinToIds(getAttributeSql(attrQuery));
-    wrapped = substitutePartitionKeys(wrapped, attrQuery.getName() + "-getFilteredAttributeSql()");
-    if (!sort)
-      return wrapped;
-
-    final var cols = getSortingColumns()
-      .stream()
-      .filter(spec -> spec.getItem() instanceof QueryColumnAttributeField)
-      .iterator();
-
-    if (!cols.hasNext())
-      return wrapped;
 
-    final var out = new StringBuilder(wrapped).append("\nORDER BY\n inq.");
-
-    boolean first = true;
-    while (cols.hasNext()) {
-      final var spec = cols.next();
-
-      if (!first)
-        out.append("\n, inq.");
+    // get attribute SQL joined to IDs
+    String wrapped = joinToIds(getAttributeSql(attrQuery));
 
-      out.append(spec.getItemName())
-        .append(' ')
-        .append(spec.getDirection().toString());
-      first=false;
-    }
+    // substitute partition keys
+    wrapped = substitutePartitionKeys(wrapped, attrQuery.getName() + "-getFilteredAttributeSql()");
 
-    return out.toString();
+    // if asked to sort, append generated order-by clause
+    return !sort ? wrapped :
+      wrapped + getOrderByClause(_question.getRecordClass().getPrimaryKeyDefinition(),
+          _sortingMap, _question.getAttributeFieldMap(), Optional.of("inq."));
   }
 
   public String getAttributeSql(Query attributeQuery) throws WdkModelException {
@@ -772,11 +754,14 @@ private void prepareSortingSqls(Map<String, String> sqls, Collection<String> ord
     LOG.debug("AnswerValue: prepareSortingSqls(): sorting map: " + _sortingMap); //e.g.: {primary_key=true}
     final String idQueryNameStub = "answer_id_query";
     queryNames.put(idQueryNameStub, "idq");
+
     for (String fieldName : _sortingMap.keySet()) {
       AttributeField field = fields.get(fieldName);
       if (field == null) continue;
       boolean ascend = _sortingMap.get(fieldName);
+
       Map<String, ColumnAttributeField> dependents = field.getColumnAttributeFields();
+
       for (ColumnAttributeField dependent : dependents.values()) {
 
         // set default values for PK and simple columns
@@ -869,12 +854,76 @@ public Optional<String> getResultMessage() throws WdkModelException {
     return _idsQueryInstance.getResultMessage();
   }
 
-  public Map<String, Boolean> getSortingMap() {
+  public void setSortByIdAttribute() {
+    String idAttribute = _question.getRecordClass().getIdAttributeField().getName();
+    _sortingMap.clear();
+    _sortingMap.put(idAttribute, true);
+  }
+
+  /**
+   * @return the requested column names to sort and a boolean indication sort direction (true = ascending)
+   */
+  public LinkedHashMap<String, Boolean> getSortingMap() {
     return new LinkedHashMap<>(_sortingMap);
   }
 
-  public List<SortDirectionSpec<AttributeField>> getSortingColumns() {
-    return SortDirectionSpec.convertSorting(_sortingMap, _question.getAttributeFieldMap());
+  /**
+   * Converts the passed sorting columns into those used to perform the actual sort; note
+   * these columns could be dependent columns (for derived atrributes) and/or may have been
+   * mapped to proxy sort columns and may not match the column names returned by getSortingMap().
+   *
+   * Note 1: some of this logic is repeated in prepareSortingSqls() and a combination of
+   *    RecorStreamFactory::requiresExactlyOneAttrQuery and FileBasedRecordStream::getRequiredColumnAttributeFields.
+   *
+   * Note 2: We append primary key columns to the end to ensure consistent paging; this
+   *    means if you want to avoid runtime by not sorting at all (PKs included) if
+   *    _sortingMap is empty, do NOT call this method.  It will always return a sort.
+   * @throws WdkModelException 
+   */
+  public static String getOrderByClause(PrimaryKeyDefinition pk, Map<String, Boolean> sortingColumns,
+       Map<String, AttributeField> attributeFieldMap, Optional<String> tableAlias) throws WdkModelException {
+
+    String aliasStr = tableAlias.map(a -> a.endsWith(".") ? a : a + ".").orElse("");
+
+    List<SortDirectionSpec<AttributeField>> baseSorts =
+        SortDirectionSpec.convertSorting(sortingColumns, attributeFieldMap);
+
+    Map<String, String> orderClauses = new LinkedHashMap<>();
+
+    for (SortDirectionSpec<AttributeField> spec : baseSorts) {
+
+      for (ColumnAttributeField dependedField: spec.getItem().getColumnAttributeFields().values()) {
+
+        if (dependedField instanceof QueryColumnAttributeField) {
+
+          QueryColumnAttributeField queryField = (QueryColumnAttributeField)dependedField;
+          Column column = queryField.getColumn();
+
+          // use custom sorting column if specified
+          String columnName = Optional
+              .ofNullable(column.getSortingColumn())
+              .orElse(column.getName());
+
+          orderClauses.putIfAbsent(columnName, column.isIgnoreCase()
+              ? "lower(" + aliasStr + columnName + ") " + spec.getDirection()
+              : aliasStr + columnName + " " + spec.getDirection());
+        }
+        else if (dependedField instanceof PkColumnAttributeField) {
+          orderClauses.putIfAbsent(dependedField.getName(), dependedField.getName() + " ASC");
+        }
+        else {
+          throw new IllegalStateException("Unknown subclass of ColumnAttributeField: " + dependedField.getClass().getName());
+        }
+      }
+    }
+
+    // for true stable sort, add PK cols to the end if they are not already present
+    for (String pkCol : pk.getColumnRefs()) {
+      orderClauses.putIfAbsent(pkCol, pkCol + " ASC");
+    }
+
+    // join together
+    return orderClauses.values().stream().collect(Collectors.joining(", ", "\n ORDER BY ", "\n"));
   }
 
   /**
@@ -1048,19 +1097,22 @@ public boolean entireResultRequested() {
   }
 
   public static String wrapToReturnOnlyPkAndSelectedCols(String sql,
-      RecordClass rc, Collection<QueryColumnAttributeField> fields) {
+      Question question, Collection<QueryColumnAttributeField> fields,
+      Map<String,Boolean> sortingColumns) throws WdkModelException {
 
+    PrimaryKeyDefinition pk = question.getRecordClass().getPrimaryKeyDefinition();
     List<String> cols = new ListBuilder<String>()
-      .addAll(Arrays.asList(rc.getPrimaryKeyDefinition().getColumnRefs()))
+      .addAll(Arrays.asList(pk.getColumnRefs()))
       .addAll(fields.stream().map(Field::getName).collect(Collectors.toList()))
       .toList();
-    
+
     return new StringBuilder()
       .append("/* SingleAttributeRecordStream */\nSELECT\n  ")
       .append(String.join(",\n  ", cols))
       .append("\n FROM (\n")
       .append(sql)
       .append("\n) sarsc")
+      .append(getOrderByClause(pk, sortingColumns, question.getAttributeFieldMap(), Optional.of("sarsc")))
       .toString();
   }
 

Model/src/main/java/org/gusdb/wdk/model/answer/factory/AnswerValueFactory.java

@@ -1,7 +1,6 @@
 package org.gusdb.wdk.model.answer.factory;
 
-import java.util.Collections;
-import java.util.Map;
+import java.util.LinkedHashMap;
 
 import org.gusdb.fgputil.validation.ValidObjectFactory.RunnableObj;
 import org.gusdb.wdk.model.WdkModelException;
@@ -25,7 +24,7 @@ public static AnswerValue makeAnswer(RunnableObj<AnswerSpec> validSpec, boolean
     Question question = validSpec.get().getQuestion().get();
     int pageStart = 1;
     int pageEnd = AnswerValue.UNBOUNDED_END_PAGE_INDEX;
-    AnswerValue answerValue = makeAnswer(validSpec, pageStart, pageEnd, Collections.emptyMap(), avoidCacheHit);
+    AnswerValue answerValue = makeAnswer(validSpec, pageStart, pageEnd, new LinkedHashMap<>(), avoidCacheHit);
     if (question.isFullAnswer()) {
       int resultSize = answerValue.getResultSizeFactory().getResultSize();
       if (resultSize > pageEnd)
@@ -35,7 +34,7 @@ public static AnswerValue makeAnswer(RunnableObj<AnswerSpec> validSpec, boolean
   }
 
   public static AnswerValue makeAnswer(RunnableObj<AnswerSpec> validSpec,
-      int startIndex, int endIndex, Map<String, Boolean> sortingMap, boolean avoidCacheHit) throws WdkModelException {
+      int startIndex, int endIndex, LinkedHashMap<String, Boolean> sortingMap, boolean avoidCacheHit) throws WdkModelException {
     Question question = validSpec.get().getQuestion().get();
     if (question instanceof SingleRecordQuestion) {
       return new SingleRecordAnswerValue(validSpec);

Model/src/main/java/org/gusdb/wdk/model/answer/single/SingleRecordAnswerValue.java

@@ -3,7 +3,7 @@
 import static org.gusdb.fgputil.FormatUtil.join;
 import static org.gusdb.fgputil.functional.Functions.mapToList;
 
-import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.NoSuchElementException;
 
@@ -38,7 +38,7 @@ public SingleRecordResultSizeFactory(AnswerValue answerValue) {
   private Map<String, Object> _pkMap;
 
   public SingleRecordAnswerValue(RunnableObj<AnswerSpec> validSpec) throws WdkModelException {
-    super(validSpec, 1, UNBOUNDED_END_PAGE_INDEX, Collections.EMPTY_MAP, false);
+    super(validSpec, 1, UNBOUNDED_END_PAGE_INDEX, new LinkedHashMap<>(), false);
     SingleRecordQuestion question = (SingleRecordQuestion)validSpec.get().getQuestion().get();
     SingleRecordQuestionParam param = question.getParam();
     _recordClass = question.getRecordClass();

Model/src/main/java/org/gusdb/wdk/model/answer/stream/RecordStreamFactory.java

@@ -2,6 +2,7 @@
 
 import org.apache.log4j.Logger;
 import org.gusdb.fgputil.SortDirectionSpec;
+import org.gusdb.fgputil.functional.Functions;
 import org.gusdb.wdk.model.WdkModelException;
 import org.gusdb.wdk.model.WdkUserException;
 import org.gusdb.wdk.model.answer.AnswerValue;
@@ -98,8 +99,13 @@ private static boolean requiresExactlyOneAttrQuery(
     }
     var fieldsToConsider = new ArrayList<>(fields);
     if (includeSortingColumnsInCalculation) {
-      fieldsToConsider.addAll(answerValue.getSortingColumns()
-          .stream().map(SortDirectionSpec::getItem).collect(Collectors.toList()));
+      fieldsToConsider.addAll(
+          SortDirectionSpec.convertSorting(
+              answerValue.getSortingMap(),
+              Functions.getMapFromValues(fields, AttributeField::getName))
+          .stream()
+          .map(SortDirectionSpec::getItem)
+          .collect(Collectors.toList()));
     }
     return FileBasedRecordStream.requiresExactlyOneAttributeQuery(fieldsToConsider);
   }

Model/src/main/java/org/gusdb/wdk/model/answer/stream/SingleAttributeRecordStream.java

@@ -40,8 +40,8 @@ public SingleAttributeRecordStream(
     _sql = attributeQuery == null
         ? (performSort ? _answerValue.getSortedIdSql() : _answerValue.getIdSql())
         : AnswerValue.wrapToReturnOnlyPkAndSelectedCols(
-            _answerValue.getFilteredAttributeSql(attributeQuery, performSort),
-            _answerValue.getQuestion().getRecordClass(), _requiredFields);
+            _answerValue.getFilteredAttributeSql(attributeQuery, false), // do not perform sort here; must do it in the wrapper
+            _answerValue.getQuestion(), _requiredFields, _answerValue.getSortingMap());
   }
 
   public String getSql() {

Model/src/main/java/org/gusdb/wdk/model/query/SqlQueryInstance.java

@@ -85,7 +85,7 @@ public ResultList getUncachedResultsSubstitutePartitionKeys(PartitionKeysProvide
       var sql = partitionKeysProvider.substitutePartitionKeys(getUncachedSql(), getQuery().getFullName());
 
       LOG.info("Performing the following SQL: (use debug to see SQL) " );
-      LOG.debug("Performing the following SQL: " + sql);
+      LOG.info("Performing the following SQL: " + sql);
       return new SqlResultList(SqlUtils.executeQuery(
           _wdkModel.getAppDb().getDataSource(),
           sql,

Model/src/main/java/org/gusdb/wdk/model/question/Question.java

@@ -666,8 +666,8 @@ public void setQuestionSet(QuestionSet questionSet) {
    * This method is use to clone the question, excluding dynamic attributes
    */
   @Override
-  public Map<String, Boolean> getSortingAttributeMap() {
-    Map<String, Boolean> map = new LinkedHashMap<>();
+  public LinkedHashMap<String, Boolean> getSortingAttributeMap() {
+    LinkedHashMap<String, Boolean> map = new LinkedHashMap<>();
 
     for (String attrName : _defaultSortingMap.keySet()) {
       map.put(attrName, _defaultSortingMap.get(attrName));

Model/src/main/java/org/gusdb/wdk/model/record/RecordClass.java

@@ -1427,8 +1427,8 @@ public Map<String, AttributeField> getSummaryAttributeFieldMap() {
   }
 
   @Override
-  public Map<String, Boolean> getSortingAttributeMap() {
-    Map<String, Boolean> map = new LinkedHashMap<>();
+  public LinkedHashMap<String, Boolean> getSortingAttributeMap() {
+    LinkedHashMap<String, Boolean> map = new LinkedHashMap<>();
     int count = 0;
     for (String attrName : defaultSortingMap.keySet()) {
       map.put(attrName, defaultSortingMap.get(attrName));
@@ -1445,8 +1445,8 @@ public Map<String, Boolean> getSortingAttributeMap() {
     return map;
   }
 
-  public Map<String, Boolean> getIdSortingAttributeMap() {
-    return new MapBuilder<String, Boolean>(new LinkedHashMap<>())
+  public LinkedHashMap<String, Boolean> getIdSortingAttributeMap() {
+    return (LinkedHashMap<String,Boolean>)new MapBuilder<String, Boolean>(new LinkedHashMap<>())
         .put(idAttributeField.getName(), true).toMap();
   }
 

Model/src/main/java/org/gusdb/wdk/model/report/reporter/AbstractTabularReporter.java

@@ -141,6 +141,13 @@ public void write(OutputStream out) throws WdkModelException {
     }
   }
 
+  @Override
+  protected RecordStream getRecords() throws WdkModelException {
+    // tabular reporters should always be sorted by the ID attribute; set sort before getting records
+    _baseAnswer.setSortByIdAttribute();
+    return super.getRecords();
+  }
+
   /**
    * Ensures values are uniformly formatted across output types
    *

Model/src/main/java/org/gusdb/wdk/model/report/reporter/TableTabularReporter.java

@@ -65,6 +65,7 @@ public RecordStream getRecords() throws WdkModelException {
         throw new WdkModelException(e.getMessage(), e);
       }
     } */
+    _baseAnswer.setSortByIdAttribute();
     RecordClass recordClass = _baseAnswer.getQuestion().getRecordClass();
     if (idAttributeContainsNonPkFields(recordClass)) {
       // need to use FileBasedRecordStream to support both this table and any needed attributes