-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
Show filter condition #87
Show filter condition #87
Conversation
query_plan_test.go
Outdated
t.Run(test.title, func(t *testing.T) { | ||
b, err := ioutil.ReadFile(test.file) | ||
if err != nil { | ||
t.Error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be t.Fatal
as it can't continue the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a05abe8
query_plan_test.go
Outdated
var plan pb.QueryPlan | ||
err = protojson.Unmarshal(b, &plan) | ||
if err != nil { | ||
t.Error(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same to the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a05abe8
query_plan.go
Outdated
if child.DisplayName != "Function" || !(cl.GetType() == "Residual Condition" || cl.GetType() == "Seek Condition" || cl.GetType() == "Split Range") { | ||
// Known predicates are Condition(Filter) or Seek Condition/Residual Condition(FilterScan) or Split Range(Distributed Union). | ||
// Agg is a Function but not a predicate. | ||
if child.DisplayName != "Function" || (!strings.HasSuffix(cl.GetType(), "Condition") && cl.GetType() != "Split Range") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, what is the reason to have condition child.DisplayName != "Function"
? If I understand correctly, Function
operator is not visible in our query plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Predicates are subset of Function
operators which returns bool so Function
are not visible as nodes of the operator tree but needs to be visible as predicates.
Although, there are no return type information in PlanNode
so it is whitelisted by the type name of ChildLink
which link to the Function
operator as predicates.
Currently, I don't yet see a child link like which type is *Condition
and child is not Function
so only ChildLink.type
is needed information to distinguish predicates but I think that to be Function
is a necessary condition to be a predicate so I require it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract isPredicate
function in 876f838
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense. Thank you for clarifying it!
@@ -0,0 +1,277 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Currently, Seek Condition/Residual Condition predicates of FilterScan operator are shown but Condition predicate of Filter operator is not shown.
Showing them helps to understand query executions.
This PR fix to show Filter's Condition.
Note: It adds testdata directory which contains QueryPlan as JSON.
Before
After