Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable AggregateProjectMergeRule #15003

Closed
wants to merge 14 commits into from
+107 −51
Closed

Enable AggregateProjectMergeRule #15003

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
af9e3ed
Enable already passing tests in DecoupledPlanningCalciteQueryTest
kgyrtkirk Sep 15, 2023
b22bd26
empty
kgyrtkirk Sep 15, 2023
1644321
add transposerule; fix prefix
kgyrtkirk Sep 18, 2023
38bfec3
accept a few more
kgyrtkirk Sep 18, 2023
2b6385d
add projectmerge;aggrproject
kgyrtkirk Sep 18, 2023
eaa5a20
fix order
kgyrtkirk Sep 18, 2023
4ae44c2
Merge remote-tracking branch 'apache/master' into decouple-test-fixes-2
kgyrtkirk Jan 17, 2024
8c1b235
not needed
kgyrtkirk Jan 17, 2024
206e556
undo postAgg name change for now; fix 1 test
kgyrtkirk Jan 17, 2024
e8498f8
some changes
kgyrtkirk Jan 18, 2024
8ceb566
kinda back to orig diffs
kgyrtkirk Jan 18, 2024
2e99c01
some changes
kgyrtkirk Jan 18, 2024
0fd791c
x
kgyrtkirk Jan 18, 2024
454afd3
more trials
kgyrtkirk Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ public class CalciteRulesManager
CoreRules.FILTER_VALUES_MERGE,
CoreRules.PROJECT_FILTER_VALUES_MERGE,
CoreRules.PROJECT_VALUES_MERGE,
CoreRules.PROJECT_MERGE,
CoreRules.AGGREGATE_PROJECT_MERGE,
CoreRules.SORT_PROJECT_TRANSPOSE,
CoreRules.AGGREGATE_VALUES
);

Expand Down
2 changes: 1 addition & 1 deletion sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ private static Sorting computeSorting(

projection = Projection.preAggregation(sortProject, plannerContext, rowSignature, virtualColumnRegistry);
} else {
projection = Projection.postAggregation(sortProject, plannerContext, rowSignature, "s");
projection = Projection.postAggregation(sortProject, plannerContext, rowSignature, "p");
}

return Sorting.create(orderBys, offsetLimit, projection);
Expand Down
14 changes: 7 additions & 7 deletions sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6706,8 +6706,8 @@ public void testExactCountDistinctWithGroupingAndOtherAggregators()
.setInterval(querySegmentSpec(Filtration.eternity()))
.setGranularity(Granularities.ALL)
.setDimensions(dimensions(
new DefaultDimensionSpec("dim2", "d0"),
new DefaultDimensionSpec("dim1", "d1")
new DefaultDimensionSpec("dim1", "d0"),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grouping columns should come first

new DefaultDimensionSpec("dim2", "d1")
))
.setAggregatorSpecs(aggregators(new LongSumAggregatorFactory("a0", "cnt")))
.setContext(QUERY_CONTEXT_DEFAULT)
Expand All @@ -6716,12 +6716,12 @@ public void testExactCountDistinctWithGroupingAndOtherAggregators()
)
.setInterval(querySegmentSpec(Filtration.eternity()))
.setGranularity(Granularities.ALL)
.setDimensions(dimensions(new DefaultDimensionSpec("d0", "_d0")))
.setDimensions(dimensions(new DefaultDimensionSpec("d1", "_d0")))
.setAggregatorSpecs(aggregators(
new LongSumAggregatorFactory("_a0", "a0"),
new FilteredAggregatorFactory(
new CountAggregatorFactory("_a1"),
notNull("d1")
notNull("d0")
)
))
.setContext(QUERY_CONTEXT_DEFAULT)
Expand Down Expand Up @@ -7877,8 +7877,8 @@ public void testGroupBySortPushDown()
.setGranularity(Granularities.ALL)
.setDimensions(
dimensions(
new DefaultDimensionSpec("dim2", "d0"),
new DefaultDimensionSpec("dim1", "d1")
new DefaultDimensionSpec("dim1", "d0"),
new DefaultDimensionSpec("dim2", "d1")
)
)
.setAggregatorSpecs(
Expand All @@ -7889,7 +7889,7 @@ public void testGroupBySortPushDown()
.setLimitSpec(
new DefaultLimitSpec(
ImmutableList.of(
new OrderByColumnSpec("d1", OrderByColumnSpec.Direction.ASCENDING)
new OrderByColumnSpec("d0", OrderByColumnSpec.Direction.ASCENDING)
),
4
)
Expand Down
95 changes: 0 additions & 95 deletions sql/src/test/java/org/apache/druid/sql/calcite/DecoupledPlanningCalciteQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import org.apache.druid.sql.calcite.planner.PlannerConfig;
import org.apache.druid.sql.calcite.util.SqlTestFramework;
import org.junit.Ignore;
import org.junit.Test;

public class DecoupledPlanningCalciteQueryTest extends CalciteQueryTest
{
Expand Down Expand Up @@ -227,20 +226,6 @@ public void testGroupByTimeFloorAndDimOnGroupByTimeFloorAndDim()

}

@Override
@Ignore
public void testPostAggWithTimeseries()
{

}

@Override
@Ignore
public void testPostAggWithTopN()
{

}

@Override
@Ignore
public void testRequireTimeConditionPositive()
Expand All @@ -254,77 +239,20 @@ public void testRequireTimeConditionSemiJoinNegative()

}

@Override
@Ignore
public void testSubqueryTypeMismatchWithLiterals()
{

}

@Override
@Ignore
public void testTimeseriesQueryWithEmptyInlineDatasourceAndGranularity()
{

}

@Override
@Ignore
public void testGroupBySortPushDown()
{

}

@Override
@Ignore
public void testGroupingWithNullInFilter()
{

}

@Override
@Ignore
@Test
public void testStringAggExpressionNonConstantSeparator()
{

}

@Override
@Ignore
public void testOrderByAlongWithInternalScanQuery()
{

}

@Override
@Ignore
public void testSortProjectAfterNestedGroupBy()
{

}

@Override
@Ignore
public void testOrderByAlongWithInternalScanQueryNoDistinct()
{

}

@Override
@Ignore
public void testNestedGroupBy()
{

}

@Override
@Ignore
public void testQueryWithSelectProjectAndIdentityProjectDoesNotRename()
{

}

@Override
@Ignore
public void testFilterOnCurrentTimestampWithIntervalArithmetic()
Expand All @@ -337,29 +265,6 @@ public void testFilterOnCurrentTimestampWithIntervalArithmetic()
public void testFilterOnCurrentTimestampOnView()
{

}
// When run through decoupled, it expects
// dimensions=[DefaultDimensionSpec{dimension='dim2', outputName='d0', outputType='STRING'},
// DefaultDimensionSpec{dimension='dim1', outputName='d1', outputType='STRING'}]
//
// but gets
// dimensions=[DefaultDimensionSpec{dimension='dim1', outputName='d0', outputType='STRING'},
// DefaultDimensionSpec{dimension='dim2', outputName='d1', outputType='STRING'}]
//
// The change in the ordering fails the query plan exact match. This needs to be revisited
// when we make more advancements into the decoupled planner
@Override
@Ignore
public void testExactCountDistinctWithGroupingAndOtherAggregators()
{

}

@Override
@Ignore
public void testTopNWithSelectProjections()
{

}

@Override
Expand Down