kgyrtkirk commented on code in PR #16738:
URL: https://github.com/apache/druid/pull/16738#discussion_r1678871426


##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java:
##########
@@ -724,4 +729,20 @@ public void testOverWindow()
         ))
         .run();
   }
+
+  @Test
+  public void testStddevNotSupportedOverWindow()
+  {
+    assumeFeatureAvailable(EngineFeature.WINDOW_FUNCTIONS);
+
+    DruidException e = assertThrows(
+        DruidException.class,
+        () -> testBuilder()
+            
.queryContext(ImmutableMap.of(PlannerContext.CTX_ENABLE_WINDOW_FNS, true))
+            .sql("SELECT stddev(m1) OVER () from numfoo")
+            .run()
+    );
+
+    assertTrue(e.getMessage().contains("Aggregation [STDDEV] is currently not 
supported for window functions"));

Review Comment:
   `assertTrue` in this form doesn't give much to go on...
   either pass the full `e.getMessage()` as the error description; use 
`assertThat` like in the other tests
   
   



##########
extensions-core/stats/src/test/java/org/apache/druid/query/aggregation/variance/sql/VarianceSqlAggregatorTest.java:
##########
@@ -724,4 +729,20 @@ public void testOverWindow()
         ))
         .run();
   }
+
+  @Test
+  public void testStddevNotSupportedOverWindow()

Review Comment:
   I see why this test is here; I was looking for an agg in the `sql` module to 
move it there...
   
   I've bumped into a line where finalize is set to [false 
here](https://github.com/apache/druid/blob/6cf6838eb974634ce5ae664d5ca877d8c804036a/sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java#L163)
 the comment is not entirely accurate; I think the window aggs do finalize now  
[here](https://github.com/apache/druid/blob/6cf6838eb974634ce5ae664d5ca877d8c804036a/processing/src/main/java/org/apache/druid/query/rowsandcols/semantic/DefaultFramedOnHeapAggregatable.java#L315)
 so that comment should be removed or updated



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org

Reply via email to