Skip to content

allow optimizing sql expressions and virtual columns #12241

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

Merged
clintropolis merged 8 commits into from
Feb 9, 2022
Merged

allow optimizing sql expressions and virtual columns #12241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f247942
rework sql planner expression and virtual column handling
clintropolis Feb 5, 2022
5c899d2
simplify a bit
clintropolis Feb 5, 2022
cd81e8f
add back and deprecate old methods, more tests, fix multi-value strin…
clintropolis Feb 8, 2022
f29befa
spotbugs
clintropolis Feb 8, 2022
53af183
fix bugs with multi-value string array expression handling
clintropolis Feb 9, 2022
90fd30c
javadocs and adjust test
clintropolis Feb 9, 2022
f53b46b
better
clintropolis Feb 9, 2022
c5505eb
fix tests
clintropolis Feb 9, 2022
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
5 changes: 5 additions & 0 deletions core/src/main/java/org/apache/druid/math/expr/Expr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ default boolean isNullLiteral()
return false;
}

default boolean isIdentifier()
{
return false;
}

/**
* Returns the value of expr if expr is a literal, or throws an exception otherwise.
*
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/java/org/apache/druid/math/expr/IdentifierExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ public String getBinding()
return binding;
}

@Override
public boolean isIdentifier()
{
return true;
}

@Nullable
@Override
public String getIdentifierIfIdentifier()
Expand Down
6 changes: 6 additions & 0 deletions core/src/test/java/org/apache/druid/math/expr/ParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,7 @@ private void validateLiteral(String expr, ExpressionType type, Object expected,
Expr parsedFlat = Parser.parse(expr, ExprMacroTable.nil(), true);
Assert.assertTrue(parsed.isLiteral());
Assert.assertTrue(parsedFlat.isLiteral());
Assert.assertFalse(parsed.isIdentifier());
Assert.assertEquals(type, parsed.getOutputType(emptyBinding));
Assert.assertEquals(type, parsedFlat.getOutputType(emptyBinding));
Assert.assertEquals(expected, parsed.getLiteralValue());
Expand Down Expand Up @@ -770,6 +771,11 @@ private void validateParser(
)
{
final Expr parsed = Parser.parse(expression, ExprMacroTable.nil());
if (parsed instanceof IdentifierExpr) {
Assert.assertTrue(parsed.isIdentifier());
} else {
Assert.assertFalse(parsed.isIdentifier());
}
final Expr.BindingAnalysis deets = parsed.analyzeInputs();
Assert.assertEquals(expression, expected, parsed.toString());
Assert.assertEquals(expression, identifiers, deets.getRequiredBindingsList());
Expand Down
8 changes: 3 additions & 5 deletions ...ch/src/main/java/org/apache/druid/query/aggregation/tdigestsketch/TDigestSketchUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
import com.tdunning.math.stats.MergingDigest;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import org.apache.druid.sql.calcite.expression.DruidExpression;
import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry;

Expand Down Expand Up @@ -88,16 +86,16 @@ public static boolean matchingAggregatorFactoryExists(
{
// Check input for equivalence.
final boolean inputMatches;
final VirtualColumn virtualInput =
virtualColumnRegistry.findVirtualColumns(factory.requiredFields())
final DruidExpression virtualInput =

@clintropolis clintropolis Feb 8, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this pattern is done in a few places, and i think could be done more efficiently, but I avoided changing the logic at this point to reduce the number of moving parts since this PR has become a bit large

virtualColumnRegistry.findVirtualColumnExpressions(factory.requiredFields())
.stream()
.findFirst()
.orElse(null);

if (virtualInput == null) {
inputMatches = input.isDirectColumnAccess() && input.getDirectColumn().equals(factory.getFieldName());
} else {
inputMatches = ((ExpressionVirtualColumn) virtualInput).getExpression().equals(input.getExpression());
inputMatches = virtualInput.equals(input);
}
return inputMatches && compression == factory.getCompression();
}
Expand Down
10 changes: 2 additions & 8 deletions .../apache/druid/query/aggregation/tdigestsketch/sql/TDigestGenerateSketchSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.tdigestsketch.TDigestSketchAggregatorFactory;
import org.apache.druid.query.aggregation.tdigestsketch.TDigestSketchUtils;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
Expand Down Expand Up @@ -135,16 +134,11 @@ public Aggregation toDruidAggregation(
compression
);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
input,
ColumnType.FLOAT
);
aggregatorFactory = new TDigestSketchAggregatorFactory(
aggName,
virtualColumn.getOutputName(),
compression
);
aggregatorFactory = new TDigestSketchAggregatorFactory(aggName, virtualColumnName, compression);
}

return Aggregation.create(aggregatorFactory);
Expand Down
10 changes: 2 additions & 8 deletions .../apache/druid/query/aggregation/tdigestsketch/sql/TDigestSketchQuantileSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.apache.druid.query.aggregation.tdigestsketch.TDigestSketchAggregatorFactory;
import org.apache.druid.query.aggregation.tdigestsketch.TDigestSketchToQuantilePostAggregator;
import org.apache.druid.query.aggregation.tdigestsketch.TDigestSketchUtils;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
Expand Down Expand Up @@ -154,16 +153,11 @@ public Aggregation toDruidAggregation(
compression
);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
input,
ColumnType.FLOAT
);
aggregatorFactory = new TDigestSketchAggregatorFactory(
sketchName,
virtualColumn.getOutputName(),
compression
);
aggregatorFactory = new TDigestSketchAggregatorFactory(sketchName, virtualColumnName, compression);
}

return Aggregation.create(
Expand Down
6 changes: 2 additions & 4 deletions ...a/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchBaseSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.apache.druid.query.aggregation.datasketches.hll.HllSketchMergeAggregatorFactory;
import org.apache.druid.query.dimension.DefaultDimensionSpec;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.column.ValueType;
Expand Down Expand Up @@ -143,12 +142,11 @@ public Aggregation toDruidAggregation(
if (columnArg.isDirectColumnAccess()) {
dimensionSpec = columnArg.getSimpleExtraction().toDimensionSpec(null, inputType);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
columnArg,
dataType
);
dimensionSpec = new DefaultDimensionSpec(virtualColumn.getOutputName(), null, inputType);
dimensionSpec = new DefaultDimensionSpec(virtualColumnName, null, inputType);
}

aggregatorFactory = new HllSketchBuildAggregatorFactory(
Expand Down
13 changes: 5 additions & 8 deletions ...uery/aggregation/datasketches/quantiles/sql/DoublesSketchApproxQuantileSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,8 @@
import org.apache.druid.query.aggregation.datasketches.quantiles.DoublesSketchAggregatorFactory;
import org.apache.druid.query.aggregation.datasketches.quantiles.DoublesSketchToQuantilePostAggregator;
import org.apache.druid.query.aggregation.post.FieldAccessPostAggregator;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.aggregation.Aggregations;
import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
Expand Down Expand Up @@ -135,16 +133,16 @@ public Aggregation toDruidAggregation(

// Check input for equivalence.
final boolean inputMatches;
final VirtualColumn virtualInput =
virtualColumnRegistry.findVirtualColumns(theFactory.requiredFields())
final DruidExpression virtualInput =
virtualColumnRegistry.findVirtualColumnExpressions(theFactory.requiredFields())
.stream()
.findFirst()
.orElse(null);

if (virtualInput == null) {
inputMatches = input.isDirectColumnAccess() && input.getDirectColumn().equals(theFactory.getFieldName());
} else {
inputMatches = ((ExpressionVirtualColumn) virtualInput).getExpression().equals(input.getExpression());
inputMatches = virtualInput.equals(input);
}

final boolean matches = inputMatches
Expand Down Expand Up @@ -177,14 +175,13 @@ public Aggregation toDruidAggregation(
getMaxStreamLengthFromQueryContext(plannerContext.getQueryContext())
);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
input,
ColumnType.FLOAT
);
aggregatorFactory = new DoublesSketchAggregatorFactory(
histogramName,
virtualColumn.getOutputName(),
virtualColumnName,
k,
getMaxStreamLengthFromQueryContext(plannerContext.getQueryContext())
);
Expand Down
6 changes: 2 additions & 4 deletions .../druid/query/aggregation/datasketches/quantiles/sql/DoublesSketchObjectSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.query.aggregation.AggregatorFactory;
import org.apache.druid.query.aggregation.datasketches.quantiles.DoublesSketchAggregatorFactory;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
Expand Down Expand Up @@ -117,14 +116,13 @@ public Aggregation toDruidAggregation(
DoublesSketchApproxQuantileSqlAggregator.getMaxStreamLengthFromQueryContext(plannerContext.getQueryContext())
);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
input,
ColumnType.FLOAT
);
aggregatorFactory = new DoublesSketchAggregatorFactory(
histogramName,
virtualColumn.getOutputName(),
virtualColumnName,
k,
DoublesSketchApproxQuantileSqlAggregator.getMaxStreamLengthFromQueryContext(plannerContext.getQueryContext())
);
Expand Down
6 changes: 2 additions & 4 deletions ...g/apache/druid/query/aggregation/datasketches/theta/sql/ThetaSketchBaseSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.apache.druid.query.aggregation.datasketches.theta.SketchMergeAggregatorFactory;
import org.apache.druid.query.dimension.DefaultDimensionSpec;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.column.ValueType;
Expand Down Expand Up @@ -123,12 +122,11 @@ public Aggregation toDruidAggregation(
if (columnArg.isDirectColumnAccess()) {
dimensionSpec = columnArg.getSimpleExtraction().toDimensionSpec(null, inputType);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
columnArg,
dataType
);
dimensionSpec = new DefaultDimensionSpec(virtualColumn.getOutputName(), null, inputType);
dimensionSpec = new DefaultDimensionSpec(virtualColumnName, null, inputType);
}

aggregatorFactory = new SketchMergeAggregatorFactory(
Expand Down
6 changes: 3 additions & 3 deletions ...a/org/apache/druid/query/aggregation/datasketches/hll/sql/HllSketchSqlAggregatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,14 +479,14 @@ public void testHllSketchPostAggs() throws Exception
new FieldAccessPostAggregator("p1", "a1"),
new HllSketchToEstimatePostAggregator("p3", new FieldAccessPostAggregator("p2", "a0"), false),
new HllSketchToEstimatePostAggregator("p5", new FieldAccessPostAggregator("p4", "a0"), false),
new ExpressionPostAggregator("p6", "(p5 + 1)", null, TestExprMacroTable.INSTANCE),
new ExpressionPostAggregator("p6", "(\"p5\" + 1)", null, TestExprMacroTable.INSTANCE),
new HllSketchToEstimatePostAggregator("p8", new FieldAccessPostAggregator("p7", "a2"), false),
new HllSketchToEstimatePostAggregator(
"p10",
new FieldAccessPostAggregator("p9", "a0"),
false
),
new ExpressionPostAggregator("p11", "abs(p10)", null, TestExprMacroTable.INSTANCE),
new ExpressionPostAggregator("p11", "abs(\"p10\")", null, TestExprMacroTable.INSTANCE),
new HllSketchToEstimateWithBoundsPostAggregator(
"p13",
new FieldAccessPostAggregator("p12", "a0"),
Expand All @@ -500,7 +500,7 @@ public void testHllSketchPostAggs() throws Exception
new FieldAccessPostAggregator("p16", "a3"),
new HllSketchToStringPostAggregator("p18", new FieldAccessPostAggregator("p17", "a0")),
new HllSketchToStringPostAggregator("p20", new FieldAccessPostAggregator("p19", "a0")),
new ExpressionPostAggregator("p21", "upper(p20)", null, TestExprMacroTable.INSTANCE),
new ExpressionPostAggregator("p21", "upper(\"p20\")", null, TestExprMacroTable.INSTANCE),
new HllSketchToEstimatePostAggregator("p23", new FieldAccessPostAggregator("p22", "a0"), true)
)
)
Expand Down
8 changes: 4 additions & 4 deletions ...he/druid/query/aggregation/datasketches/quantiles/sql/DoublesSketchSqlAggregatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ public void testDoublesSketchPostAggs() throws Exception
),
new ExpressionPostAggregator(
"p3",
"(p2 + 1000)",
"(\"p2\" + 1000)",

@imply-cheddar imply-cheddar Feb 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Naive question, but is this test change indicative of a change in what SQL is required to be written? I.e. might some things that didn't require quotes previously suddenly require quotes?

@clintropolis clintropolis Feb 9, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no, I think it was potentially a bug that the identifiers were not always being escaped in expression post-aggs, that has been fixed by the changes in how we build the native string expressions. humans are still allowed to reference identifiers without the double quotes, just the expressions we generate from SQL will always be escaped with them

null,
TestExprMacroTable.INSTANCE
),
Expand All @@ -578,7 +578,7 @@ public void testDoublesSketchPostAggs() throws Exception
),
new ExpressionPostAggregator(
"p6",
"(p5 + 1000)",
"(\"p5\" + 1000)",
null,
TestExprMacroTable.INSTANCE
),
Expand All @@ -590,7 +590,7 @@ public void testDoublesSketchPostAggs() throws Exception
),
0.5f
),
new ExpressionPostAggregator("p9", "abs(p8)", null, TestExprMacroTable.INSTANCE),
new ExpressionPostAggregator("p9", "abs(\"p8\")", null, TestExprMacroTable.INSTANCE),
new DoublesSketchToQuantilesPostAggregator(
"p11",
new FieldAccessPostAggregator(
Expand Down Expand Up @@ -633,7 +633,7 @@ public void testDoublesSketchPostAggs() throws Exception
),
new ExpressionPostAggregator(
"p20",
"replace(replace(p19,'HeapCompactDoublesSketch','HeapUpdateDoublesSketch'),"
"replace(replace(\"p19\",'HeapCompactDoublesSketch','HeapUpdateDoublesSketch'),"
+ "'Combined Buffer Capacity : 6',"
+ "'Combined Buffer Capacity : 8')",
null,
Expand Down
19 changes: 8 additions & 11 deletions .../src/main/java/org/apache/druid/query/aggregation/bloom/sql/BloomFilterSqlAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,8 @@
import org.apache.druid.query.dimension.DefaultDimensionSpec;
import org.apache.druid.query.dimension.DimensionSpec;
import org.apache.druid.query.dimension.ExtractionDimensionSpec;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.aggregation.SqlAggregator;
import org.apache.druid.sql.calcite.expression.DruidExpression;
Expand Down Expand Up @@ -114,10 +112,10 @@ public Aggregation toDruidAggregation(

// Check input for equivalence.
final boolean inputMatches;
final VirtualColumn virtualInput = virtualColumnRegistry.findVirtualColumns(theFactory.requiredFields())
.stream()
.findFirst()
.orElse(null);
final DruidExpression virtualInput = virtualColumnRegistry.findVirtualColumnExpressions(theFactory.requiredFields())
.stream()
.findFirst()
.orElse(null);
if (virtualInput == null) {
if (input.isDirectColumnAccess()) {
inputMatches =
Expand All @@ -128,7 +126,7 @@ public Aggregation toDruidAggregation(
input.getSimpleExtraction().getExtractionFn().equals(theFactory.getField().getExtractionFn());
}
} else {
inputMatches = ((ExpressionVirtualColumn) virtualInput).getExpression().equals(input.getExpression());
inputMatches = virtualInput.equals(input);
}

final boolean matches = inputMatches && theFactory.getMaxNumEntries() == maxNumEntries;
Expand Down Expand Up @@ -161,14 +159,13 @@ public Aggregation toDruidAggregation(
input.getSimpleExtraction().getExtractionFn()
);
} else {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
input,
inputOperand.getType()
);
spec = new DefaultDimensionSpec(
virtualColumn.getOutputName(),
StringUtils.format("%s:%s", name, virtualColumn.getOutputName())
virtualColumnName,
StringUtils.format("%s:%s", name, virtualColumnName)
);
}

Expand Down
8 changes: 3 additions & 5 deletions ...filter/src/main/java/org/apache/druid/query/filter/sql/BloomFilterOperatorConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.apache.druid.query.filter.BloomKFilter;
import org.apache.druid.query.filter.BloomKFilterHolder;
import org.apache.druid.query.filter.DimFilter;
import org.apache.druid.segment.VirtualColumn;
import org.apache.druid.segment.column.RowSignature;
import org.apache.druid.sql.calcite.expression.DirectOperatorConversion;
import org.apache.druid.sql.calcite.expression.DruidExpression;
Expand Down Expand Up @@ -104,16 +103,15 @@ public DimFilter toDruidFilter(
null
);
} else if (virtualColumnRegistry != null) {
VirtualColumn virtualColumn = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
plannerContext,
String virtualColumnName = virtualColumnRegistry.getOrCreateVirtualColumnForExpression(
druidExpression,
operands.get(0).getType()
);
if (virtualColumn == null) {
if (virtualColumnName == null) {
return null;
}
return new BloomDimFilter(
virtualColumn.getOutputName(),
virtualColumnName,
holder,
null,
null
Expand Down
Loading