sql/sem/tree: run TestEval with vectorize=experimental_on by asubiotto · Pull Request #40790 · cockroachdb/cockroach

asubiotto · 2019-09-16T09:30:04Z

Release note: None

Release justification: Category 1 non-production code change to increase
test coverage of the vectorized execution engine.

Close #40635

cockroach-teamcity · 2019-09-16T09:30:14Z

This change is

rafiss

nice!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

jordanlewis

This looks good, but unfortunately I'm fairly convinced (maybe 70% confident?) that this doesn't add much or any coverage to the vectorized engine. The reason is that all of the tests use constants only, but none of the vectorized engine's operators can operate on only constants - we just didn't make operators for those.

For example, if you had an eval test that said:

eval
2+2
----
4

This would attempt to make a query like select 2+2, I think. Since the tests explicitly disable constant folding, this will make a renderNode with the scalar expression 2+2, which will then... hm, I guess I don't know what it would do. Maybe it would actually work in the end - would we plan two "Datum" operators and add them all?

@asubiotto could you please check to see if any vectorized plans actually get created from the eval tests?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)

rafiss

In #40674 I noticed that adding two constant integers ended up being done in a projPlusInt64ConstInt64Op (there was one batch where every row just had a constant value), but I stopped working on that for now so I didn't figure out why.

Reviewable status: complete! 0 of 0 LGTMs obtained

jordanlewis

Oh cool! Well in that case I guess I have no objections here.

Reviewable status: complete! 0 of 0 LGTMs obtained

asubiotto · 2019-09-17T11:23:37Z

OK, I think I was a bit naive in assuming this would just work. I did a little more investigation to ensure this runs through the vectorized engine. The current integration test works by issuing the expression through QueryRow and comparing that result to the expected result. This does do constant folding (I got confused with a comment about constant folding that just serves to resolve the type of the QueryRow result). Therefore, enabling vectorized execution doesn't add any test coverage.

I'll think of something and update the PR accordingly.

asubiotto · 2019-09-17T17:13:49Z

Updated to explicitly create a NewColOperator with the unmodified expression as the post process spec. The test currently fails with an interface conversion error which I have yet to investigate.

asubiotto · 2019-09-18T09:14:26Z

Investigated the failing test but unsure how to fix this properly. The failing test is:

        --- FAIL: TestEval/vectorized/bit_array (0.00s)
            eval_test.go:53:
                testdata/eval/bit_array:176: B'11111111111111111111111110000101'::int4
                expected:
                -123

                found:
                unexpected error from the vectorized runtime: interface conversion: coldata.column is []int64, not []int32

This is caused because we type check the expression (which returns int32) to create the materializer, but the expression result comes back as an int64. What seems to happen here is that type information is getting lost when calling helper.Init in NewColOperator. The expression helper correctly type checks the expression type to an int32 here:

cockroach/pkg/sql/execinfra/expr.go

Line 75 in 4abcf22

typedExpr, err := tree.TypeCheck(expr, semaCtx, types.Any)

But then returns the constant result (-123) cast to a tree.TypedExpr:

cockroach/pkg/sql/execinfra/expr.go

Line 92 in 4abcf22

return expr.(tree.TypedExpr), nil

The vectorized flow creator then calls ResolvedType() on this expression, which returns an int64:

cockroach/pkg/sql/colexec/execplan.go

Line 1041 in 4abcf22

datumType := t.ResolvedType()

The root cause seems to be that we don't retain proper type information in processExpression because of the call to WalkExpr with a TypedExpr then cast to a TypedExpr:

cockroach/pkg/sql/execinfra/expr.go

Lines 81 to 92 in 4abcf22

    
           // Pre-evaluate constant expressions. This is necessary to avoid repeatedly 
        
           // re-evaluating constant values every time the expression is applied. 
        
           // 
        
           // TODO(solon): It would be preferable to enhance our expression serialization 
        
           // format so this wouldn't be necessary. 
        
           c := tree.MakeConstantEvalVisitor(evalCtx) 
        
           expr, _ = tree.WalkExpr(&c, typedExpr) 
        
           if err := c.Err(); err != nil { 
        
           	return nil, err 
        
           } 
        
           return expr.(tree.TypedExpr), nil

@solongordon, could you advise on the best way to fix this?

asubiotto · 2019-09-19T15:37:03Z

Skipped that test as it is a limitation of our type system that the width gets lost when evaluating an expression. RFAL.

asubiotto · 2019-09-24T08:03:33Z

Friendly ping

jordanlewis

LGTM! This is pretty cool.

Release note: None Release justification: Category 1 non-production code change to increase test coverage of the vectorized execution engine.

asubiotto · 2019-09-25T08:41:48Z

TFTR

bors r=jordanlewis,rafiss

40790: sql/sem/tree: run TestEval with vectorize=experimental_on r=jordanlewis,rafiss a=asubiotto Release note: None Release justification: Category 1 non-production code change to increase test coverage of the vectorized execution engine. Close #40635 Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>

craig · 2019-09-25T09:10:29Z

Build succeeded

GitHub CI (Cockroach)

asubiotto requested a review from rafiss September 16, 2019 09:30

asubiotto requested review from a team and jordanlewis September 16, 2019 15:08

rafiss approved these changes Sep 16, 2019

View reviewed changes

jordanlewis reviewed Sep 16, 2019

View reviewed changes

rafiss reviewed Sep 16, 2019

View reviewed changes

jordanlewis reviewed Sep 16, 2019

View reviewed changes

jordanlewis approved these changes Sep 24, 2019

View reviewed changes

sql/sem/tree: run TestEval through the vectorized engine

798a757

Release note: None Release justification: Category 1 non-production code change to increase test coverage of the vectorized execution engine.

craig bot merged commit 798a757 into cockroachdb:master Sep 25, 2019

asubiotto deleted the veceval branch October 29, 2019 20:06

Conversation

asubiotto commented Sep 16, 2019

Uh oh!

cockroach-teamcity commented Sep 16, 2019

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

asubiotto commented Sep 17, 2019

Uh oh!

asubiotto commented Sep 17, 2019

Uh oh!

asubiotto commented Sep 18, 2019

Uh oh!

asubiotto commented Sep 19, 2019

Uh oh!

asubiotto commented Sep 24, 2019

Uh oh!

jordanlewis left a comment

Choose a reason for hiding this comment

Uh oh!

asubiotto commented Sep 25, 2019

Uh oh!

craig bot commented Sep 25, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants