Merge pull request #459 from CapsicoHealth/#429_improper_aggregate_ch…

…eck_for_datetime_columns

#429 improper aggregate check for datetime columns

src/tilda/enums/AggregateType.java

@@ -16,6 +16,9 @@
 
 package tilda.enums;
 
+import tilda.parsing.parts.Column;
+import tilda.parsing.parts.ViewColumn;
+
 public enum AggregateType
   {
   SUM,
@@ -50,6 +53,7 @@ public static AggregateType parse(String Str)
 
     public ColumnType getType(ColumnType T)
       {
+        // This method needs to be kept in sync with
         switch (this)
           {
             case ARRAY:
@@ -63,8 +67,10 @@ public ColumnType getType(ColumnType T)
             case COUNT:
               return ColumnType.LONG;
             case MIN:
-            case FIRST:
             case MAX:
+              if (T != ColumnType.DATETIME)
+                return T;
+            case FIRST:
             case LAST:
               return T;
             case SUM:
@@ -74,11 +80,29 @@ public ColumnType getType(ColumnType T)
                 return ColumnType.LONG;
               break;
             default:
-              throw new Error("Incomplete Switch statment: unknown ColumnType " + this.name() + ";");
+              throw new Error("Incomplete Switch statment: unknown Aggregate " + this.name() + ";");
           }
         throw new Error("Cannot do a " + name() + " aggregate on type " + T.name() + ".");
       }
 
+    public String isCompatible(ViewColumn VC)
+      {
+        try
+          {
+            getType(VC._SameAsObj.getType());
+            return null;
+          }
+        catch (Throwable T)
+          {
+            StringBuilder Str = new StringBuilder("View Column '" + VC.getFullName() + "' declares a nonsensical aggregate " + VC._Aggregate.name() + " over type " + VC._SameAsObj.getType().name() + ".");
+            if (VC._SameAsObj.getType() == ColumnType.DATETIME && (this == AggregateType.MIN || this == AggregateType.MAX))
+              Str.append(" Because of the way ZonedDateTimes are represented in the database as two columns, Min/Max are not supported as aggregates but you can use First/Last with orderBy instead to the same effect.");
+            return Str.toString();
+          }
+
+      }
+
+
     public boolean isComposable()
       {
         switch (this)
@@ -129,4 +153,17 @@ public boolean isOrderable()
           }
       }
 
+    /**
+     * Tests whether an aggregate is friendly with DateTime columns. Because DateTime columns have a companion
+     * TZ column to maintain the timezone information, aggregates that cannot be ordered essentially are not usable.
+     * This method simply wraps isOrderable() in case we later need to make some modifications, i.e., the
+     * isTimeZoneCompatible is semantically different enough from isOrderable that we didn't want to alias them.
+     * 
+     * @return
+     */
+    public boolean isZonedDateTimeCompatible()
+      {
+        return isOrderable() == true;
+      }
+
   }

src/tilda/generation/Generator.java

@@ -236,6 +236,9 @@ protected static void genTildaData(GeneratorSession G, File GenFolder, Object O)
               Out.println();
               DG.MustNotBeModified(Out, G);
               Out.println();
+              if (C.getName().equals("a9") == true)
+                LOG.debug("xxx");
+
 
               if (C._Mode != ColumnMode.CALCULATED || C._Mode == ColumnMode.CALCULATED && C._MapperDef != null)
                 {

src/tilda/parsing/parts/ViewColumn.java

@@ -187,9 +187,13 @@ public boolean Validate(ParserSession PS, View ParentView)
           {
             if ((_Aggregate = AggregateType.parse(_AggregateStr)) == null)
               return PS.AddError("View Column '" + getFullName() + "' defined an invalid 'aggregate' '" + _AggregateStr + "'.");
-            // if (_SameAsObj != null && _SameAsObj._Type == ColumnType.DATETIME)
-            // return PS.AddError("View Column '" + getFullName() + "' defined an aggregate on DATETIME column '" + _SameAsObj.getName() + "' which is not supported as timezone
-            // information would not be retrievable.");
+
+            if (_SameAsObj != null)
+              {
+                String Str = _Aggregate.isCompatible(this);
+                if (Str != null)
+                  PS.AddError(Str.toString());
+              }
           }
         if (_Aggregate == null)
           {
@@ -222,14 +226,14 @@ else if (_Distinct == true)
         if (_TypeStr != null)
           {
             _Type = new TypeDef(_TypeStr, _Size, _Precision, _Scale);
-            _Type.Validate(PS, "View Column '"+getFullName()+"'", true, false);
+            _Type.Validate(PS, "View Column '" + getFullName() + "'", true, false);
           }
         // Checking that type information is only present when expression is specified and vice-versa.
         if (TextUtil.isNullOrEmpty(_Expression) == false && _Type == null)
           PS.AddError("View Column '" + getFullName() + "' defined an 'expression' but neglected to specify type information and optionally, size.");
         if (TextUtil.isNullOrEmpty(_Expression) == true && _Type != null)
           PS.AddError("View Column '" + getFullName() + "' defined extra type/size information without an 'expression': type and size are for expressions only.");
-        
+
         return Errs == PS.getErrorCount();
       }
 
@@ -340,15 +344,15 @@ public static String PrintColumnList(List<ViewColumn> L)
       }
 
     /**
-     * A view column of type 'DATETIME' needs an extra timezone support field if the underlying column needs one, and the 
+     * A view column of type 'DATETIME' needs an extra timezone support field if the underlying column needs one, and the
      * view column is not an aggregate, and does not have an expression unless it's of type datetime.
+     * 
      * @return
      */
     public boolean needsTZ()
       {
         return (_SameAsObj == null || _SameAsObj.needsTZ() == true)
-            && _Aggregate == null
-            && (TextUtil.isNullOrEmpty(_Expression) == true || _Type._Type == ColumnType.DATETIME)
-            ;
+        && (_Aggregate == null || _Aggregate.isZonedDateTimeCompatible() == true)
+        && (TextUtil.isNullOrEmpty(_Expression) == true || _Type._Type == ColumnType.DATETIME);
       }
   }

ut/tilda/data_test/_tilda.TildaTest.json

@@ -180,7 +180,7 @@
          ,{ "sameas": "Testing.a2"       , "name":"a2Min"  , "aggregate":"MIN"    , "filter":"a2 is not null" , "coalesce":"AAA" }
          ,{ "sameas": "Testing.a2"       , "name":"a2Max"  , "aggregate":"MAX"    , "filter":"a2 is not null" , "coalesce":"ZZZ"}
          ,{ "sameas": "Testing.a9"       ,                   "aggregate":"ARRAY"  , "orderBy":["lastUpdated asc"] }
-         ,{ "sameas": "Testing.a9c"      ,                   "aggregate":"ARRAY"}
+         ,{ "sameas": "Testing.a9c"      ,                   "aggregate":"ARRAY"  }
          ,{ "sameas": "Testing.a6"       , "name":"a6First", "aggregate":"FIRST"  , "orderBy":["lastUpdated asc"] }
          ,{ "sameas": "Testing.a6"       , "name":"a6Last" , "aggregate":"LAST"   , "orderBy":["lastUpdated asc"] }
         ]

ut/tilda/data_tutorial/_tilda.TildaTutorial.json

@@ -195,9 +195,10 @@
   ,"description": "A pivoted view of SAT_01 forms"
   ,"columns":[
       { "sameas": "Form.type"    }
+     ,{ "sameas": "Form.fillDate", "joinOnly":true }
      ,{ "sameas": "Form.refnum"  , "name":"testCount"   , "aggregate":"COUNT", "distinct":true }
-     ,{ "sameas": "Form.fillDate", "name":"testFirst"   , "aggregate":"MIN"   }
-     ,{ "sameas": "Form.fillDate", "name":"testLast"    , "aggregate":"LAST"  }
+     ,{ "sameas": "Form.fillDate", "name":"testFirst"   , "aggregate":"FIRST", "orderBy":["fillDate"] }
+     ,{ "sameas": "Form.fillDate", "name":"testLast"    , "aggregate":"LAST" , "orderBy":["fillDate"] }
      ,{ "sameas": "User.refnum"  , "name":"userCount"   , "aggregate":"COUNT", "distinct":true }
      ,{ "sameas": "TestAnswer.correct"   , "name":"answerCount"          , "aggregate":"COUNT"   }
      ,{ "sameas": "TestAnswer.correct"   , "name":"answerCountCorrect"   , "aggregate":"COUNT", "filter":"correct = true"   }