You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/03/10 07:59:24 UTC
[GitHub] [calcite] vlsi commented on a change in pull request #2363: [CALCITE-4522] Sort operator returns the same cpu cost no matter the RelCollation is empty or not
vlsi commented on a change in pull request #2363:
URL: https://github.com/apache/calcite/pull/2363#discussion_r591176768
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3412,4 +3414,41 @@ public String colType(RelNode rel, int column) {
assertThat(columnOrigin.getOriginTable().getRowType().getFieldNames().get(5),
equalTo("SAL"));
}
+
+ // ----------------------------------------------------------------------
+ // Tests for SortCpuCost
+ // ----------------------------------------------------------------------
+
+ @Test void testSortCpuCostOffsetLimit() {
+ final String sql = "select ename from emp order by ename limit 5 offset 5";
+ double cpuCost = EMP_SIZE * Math.log(10) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ @Test void testSortCpuCostLimit() {
+ final String sql = "select ename from emp limit 10";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLimit0() {
+ final String sql = "select ename from emp order by ename limit 0";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLargeLimit() {
+ final String sql = "select ename from emp order by ename limit 10000";
+ double cpuCost = EMP_SIZE * Math.log(EMP_SIZE) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ private void checkCpuCost(String sql, double expected) {
+ RelNode rel = convertSql(sql);
+ assertThat("query should be sort", rel instanceof LogicalSort);
+ final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
+ RelOptPlanner planner = new VolcanoPlanner();
+ RelOptCost cost = rel.computeSelfCost(planner, mq);
+ assertThat(cost, notNullValue());
+ final double result = cost.getCpu();
+ assertThat(result, is(expected));
Review comment:
Please add a clarification message that explains why a specific value expected (like above)
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3412,4 +3414,41 @@ public String colType(RelNode rel, int column) {
assertThat(columnOrigin.getOriginTable().getRowType().getFieldNames().get(5),
equalTo("SAL"));
}
+
+ // ----------------------------------------------------------------------
+ // Tests for SortCpuCost
+ // ----------------------------------------------------------------------
+
+ @Test void testSortCpuCostOffsetLimit() {
+ final String sql = "select ename from emp order by ename limit 5 offset 5";
+ double cpuCost = EMP_SIZE * Math.log(10) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ @Test void testSortCpuCostLimit() {
+ final String sql = "select ename from emp limit 10";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLimit0() {
+ final String sql = "select ename from emp order by ename limit 0";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLargeLimit() {
+ final String sql = "select ename from emp order by ename limit 10000";
+ double cpuCost = EMP_SIZE * Math.log(EMP_SIZE) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ private void checkCpuCost(String sql, double expected) {
+ RelNode rel = convertSql(sql);
+ assertThat("query should be sort", rel instanceof LogicalSort);
Review comment:
Please improve assertion style.
The current assert would fail like `query should be sort, expected true got false` which is not really actionable. Why `expected true`? Why `query should be sort`?
I suggest the following:
1) Ensure input SQL is a part of the message
2) Prefer `instanceOf(LogicalSort.class)` matcher as it would display the actual input class and the desired one
3) It would be a great plus if you printed the query plan in case of failure. In other words, it is with adding the full plan into the message. Then it would contain both input SQL, and the actual plan which would make CI failure analysis way easier (think of poor developers who would observe the failure after an innocent planner fix)
4) As you might guess it might be expensive to compute the plan always, so JUnit5 assertion with lambdas might help here. On the other hand, three tests won't hurt much, so even "compute plan always" might be just fine
Please ensure the input SQL is a part of the error message.
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3412,4 +3414,41 @@ public String colType(RelNode rel, int column) {
assertThat(columnOrigin.getOriginTable().getRowType().getFieldNames().get(5),
equalTo("SAL"));
}
+
+ // ----------------------------------------------------------------------
+ // Tests for SortCpuCost
+ // ----------------------------------------------------------------------
+
+ @Test void testSortCpuCostOffsetLimit() {
+ final String sql = "select ename from emp order by ename limit 5 offset 5";
+ double cpuCost = EMP_SIZE * Math.log(10) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ @Test void testSortCpuCostLimit() {
+ final String sql = "select ename from emp limit 10";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLimit0() {
+ final String sql = "select ename from emp order by ename limit 0";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLargeLimit() {
+ final String sql = "select ename from emp order by ename limit 10000";
+ double cpuCost = EMP_SIZE * Math.log(EMP_SIZE) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ private void checkCpuCost(String sql, double expected) {
+ RelNode rel = convertSql(sql);
+ assertThat("query should be sort", rel instanceof LogicalSort);
+ final RelMetadataQuery mq = rel.getCluster().getMetadataQuery();
+ RelOptPlanner planner = new VolcanoPlanner();
+ RelOptCost cost = rel.computeSelfCost(planner, mq);
+ assertThat(cost, notNullValue());
Review comment:
Please add a clarification message that explains why non-null value expected (like above)
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3412,4 +3414,41 @@ public String colType(RelNode rel, int column) {
assertThat(columnOrigin.getOriginTable().getRowType().getFieldNames().get(5),
equalTo("SAL"));
}
+
+ // ----------------------------------------------------------------------
+ // Tests for SortCpuCost
+ // ----------------------------------------------------------------------
+
Review comment:
Frankly speaking, I am inclined that these markers add little value, and they go out of date quite soon
##########
File path: core/src/test/java/org/apache/calcite/test/RelMetadataTest.java
##########
@@ -3412,4 +3414,41 @@ public String colType(RelNode rel, int column) {
assertThat(columnOrigin.getOriginTable().getRowType().getFieldNames().get(5),
equalTo("SAL"));
}
+
+ // ----------------------------------------------------------------------
+ // Tests for SortCpuCost
+ // ----------------------------------------------------------------------
+
+ @Test void testSortCpuCostOffsetLimit() {
+ final String sql = "select ename from emp order by ename limit 5 offset 5";
+ double cpuCost = EMP_SIZE * Math.log(10) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ @Test void testSortCpuCostLimit() {
+ final String sql = "select ename from emp limit 10";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLimit0() {
+ final String sql = "select ename from emp order by ename limit 0";
+ checkCpuCost(sql, 0d);
+ }
+
+ @Test void testSortCpuCostLargeLimit() {
+ final String sql = "select ename from emp order by ename limit 10000";
+ double cpuCost = EMP_SIZE * Math.log(EMP_SIZE) * 4;
+ checkCpuCost(sql, cpuCost);
+ }
+
+ private void checkCpuCost(String sql, double expected) {
Review comment:
Thanks for factoring out the verification function. That rocks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org