Skip to content

expression filter support for vectorized query engines #10613

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
gianm merged 14 commits into from
Mar 16, 2021
Merged

expression filter support for vectorized query engines #10613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bdee07d
expression filter support for vectorized query engines
clintropolis Nov 27, 2020
b53ae6e
remove unused codes
clintropolis Nov 28, 2020
14c83db
more tests
clintropolis Nov 29, 2020
3737851
refactor, more tests
clintropolis Nov 30, 2020
c6cbe3f
suppress
clintropolis Dec 1, 2020
3736bed
Merge remote-tracking branch 'upstream/master' into vector-expr-filter
clintropolis Feb 8, 2021
5903fab
more
clintropolis Feb 10, 2021
711a6b5
more
clintropolis Feb 11, 2021
95bcf60
more
clintropolis Feb 19, 2021
8d29fd6
Merge remote-tracking branch 'upstream/master' into vector-expr-filter
clintropolis Feb 19, 2021
a3e5c91
oops, i was wrong
clintropolis Feb 19, 2021
406a0fa
comment
clintropolis Feb 19, 2021
e3b0a94
remove decorate, object dimension selector, more javadocs
clintropolis Mar 4, 2021
3477524
style
clintropolis Mar 5, 2021
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
12 changes: 6 additions & 6 deletions core/src/main/java/org/apache/druid/math/expr/BinaryLogicalOperatorExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public ExprType getOutputType(InputBindingInspector inspector)
@Override
public boolean canVectorize(InputBindingInspector inspector)
{
return inspector.areNumeric(left, right) && inspector.canVectorize(left, right);
Copy link
Copy Markdown
Contributor

@gianm gianm Jan 8, 2021

Choose a reason for hiding this comment

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

This opens not just STRING but also LONG_ARRAY and DOUBLE_ARRAY. Is that intentional (will numeric arrays work)?

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Jan 8, 2021

Choose a reason for hiding this comment

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

i think canVectorize will still end up false because array types/functions are not yet vectorizable, but yeah i guess array types would not be well handled here even if they were, though maybe that is the case for non-vectorized expression as well. I'll do a bit of exploration and see what up

Copy link
Copy Markdown
Contributor

@gianm gianm Jan 15, 2021

Choose a reason for hiding this comment

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

Did this exploration lead somewhere?

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Jan 15, 2021

Choose a reason for hiding this comment

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

array types do not currently implement canVectorize so will return the default false. We probably should not use canVectorize for argument validation and maybe should consider doing this explicitly when we can resolve the input types I think.

Copy link
Copy Markdown
Contributor

@gianm gianm Jan 15, 2021

Choose a reason for hiding this comment

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

Got it. IMO we shouldn't rely on the fact that the array types happen to not be vectorizable. If they suddenly became vectorizable then this method would start returning true, maybe improperly.

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Feb 19, 2021

Choose a reason for hiding this comment

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

like, I agree here, but canVectorize still feels wrong to saddle with type validation, it can explode when creating the processor if the types are nonsense

I think what might be best is to introduce a 'validate' method to expressions so that arguments can be checked up front, and maybe explode if the input is madness

Copy link
Copy Markdown
Contributor

@gianm gianm Feb 25, 2021

Choose a reason for hiding this comment

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

In that case, could you add a comment about the situation?

return inspector.canVectorize(left, right);
}

@Override
Expand Down Expand Up @@ -130,7 +130,7 @@ public ExprType getOutputType(InputBindingInspector inspector)
@Override
public boolean canVectorize(InputBindingInspector inspector)
{
return inspector.areNumeric(left, right) && inspector.canVectorize(left, right);
return inspector.canVectorize(left, right);
}

@Override
Expand Down Expand Up @@ -185,7 +185,7 @@ public ExprType getOutputType(InputBindingInspector inspector)
@Override
public boolean canVectorize(InputBindingInspector inspector)
{
return inspector.areNumeric(left, right) && inspector.canVectorize(left, right);
return inspector.canVectorize(left, right);
}

@Override
Expand Down Expand Up @@ -241,7 +241,7 @@ public ExprType getOutputType(InputBindingInspector inspector)
@Override
public boolean canVectorize(InputBindingInspector inspector)
{
return inspector.areNumeric(left, right) && inspector.canVectorize(left, right);
return inspector.canVectorize(left, right);
}

@Override
Expand Down Expand Up @@ -296,7 +296,7 @@ public ExprType getOutputType(InputBindingInspector inspector)
@Override
public boolean canVectorize(InputBindingInspector inspector)
{
return inspector.areNumeric(left, right) && inspector.canVectorize(left, right);
return inspector.canVectorize(left, right);
}

@Override
Expand Down Expand Up @@ -351,7 +351,7 @@ public ExprType getOutputType(InputBindingInspector inspector)
@Override
public boolean canVectorize(InputBindingInspector inspector)
{
return inspector.areNumeric(left, right) && inspector.canVectorize(left, right);
return inspector.canVectorize(left, right);
}

@Override
Expand Down
6 changes: 0 additions & 6 deletions core/src/main/java/org/apache/druid/math/expr/BinaryOperatorExpr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,6 @@ public BindingAnalysis analyzeInputs()
@Override
public ExprType getOutputType(InputBindingInspector inspector)
{
if (left.isNullLiteral()) {
return right.getOutputType(inspector);
}
if (right.isNullLiteral()) {
return left.getOutputType(inspector);
}
return ExprTypeConversion.operator(left.getOutputType(inspector), right.getOutputType(inspector));
}

Expand Down
Loading