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.

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

ast, parser: extract create view parameters to CreateViewStmt #55

Merged
merged 3 commits into from
Dec 3, 2018
Merged

ast, parser: extract create view parameters to CreateViewStmt #55

merged 3 commits into from
Dec 3, 2018

Conversation

AndrewDi
Copy link
Contributor

As part of view feature implement, extract create view parameters to CreateViewStmt, ref pingcap#8416

@AndrewDi
Copy link
Contributor Author

@XuHuaiyu PTAL

@XuHuaiyu
Copy link
Contributor

@AndrewDi Please create an issue in tidb repo to trace the P1 tasks.

parser_test.go Outdated
c.Assert(ok, IsTrue)
c.Assert(v.OrReplace, IsTrue)
c.Assert(v.Algorithm, Equals, ast.AlgorithmUndefined)
c.Assert(v.Definer, NotNil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check Equals?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add check for viewName, colList, view_select_stmt?

@zz-jason zz-jason changed the title Extract create view parameters to CreateViewStmt ast, parser: extract create view parameters to CreateViewStmt Nov 29, 2018
ast/ddl.go Outdated
AlgorithmTemptable
)

var AlgorithmType = map[Algorithm]string {
Copy link
Member

Choose a reason for hiding this comment

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

how about defining a String() function for Algorithm:

func (a *Algorithm) String() string {
    switch *a {
        case AlgorithmUndefined:  return "UNDEFINED"
        case AlgorithmMerge    :  return "MERGE"
        case AlgorithmTemptable:  return "TEMPTABLE"
    }
    return "WRONG_ALGORITHM"
}

Copy link
Member

Choose a reason for hiding this comment

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

Here is the benchmark result:

[jianzhang.zj:/tmp]
➜ go test -bench Benchmark -run xx . -count 5
goos: darwin
goarch: amd64
BenchmarkString-8       2000000000               0.62 ns/op
BenchmarkString-8       2000000000               0.63 ns/op
BenchmarkString-8       2000000000               0.63 ns/op
BenchmarkString-8       2000000000               0.62 ns/op
BenchmarkString-8       2000000000               0.62 ns/op
BenchmarkMapAcess-8     500000000                3.75 ns/op
BenchmarkMapAcess-8     500000000                3.79 ns/op
BenchmarkMapAcess-8     500000000                3.75 ns/op
BenchmarkMapAcess-8     500000000                3.74 ns/op
BenchmarkMapAcess-8     500000000                3.75 ns/op
PASS
ok      _/tmp   17.924s

Here is the benchmark code:

package main

import "testing"

type Algorithm int

const (
    AlgorithmUndefined Algorithm = iota
    AlgorithmMerge
    AlgorithmTemptable
)

func (a *Algorithm) String() string {
    switch *a {
    case AlgorithmUndefined:
        return "UNDEFINED"
    case AlgorithmMerge:
        return "MERGE"
    case AlgorithmTemptable:
        return "TEMPTABLE"
    }
    return "WRONG_ALGORITHM"
}

var AlgorithmType = map[Algorithm]string{
    AlgorithmUndefined: "UNDEFINED",
    AlgorithmMerge:     "MERGE",
    AlgorithmTemptable: "TEMPTABLE",
}

func BenchmarkString(b *testing.B) {
    var a Algorithm = AlgorithmUndefined
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        a.String()
    }
}

func BenchmarkMapAcess(b *testing.B) {
    var a Algorithm = AlgorithmUndefined
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        _ = AlgorithmType[a]
    }
}

ast/ddl.go Outdated

// Algorithm is VIEW's SQL AlGORITHM characteristic
// See https://dev.mysql.com/doc/refman/5.7/en/view-algorithms.html
type Algorithm int
Copy link
Member

Choose a reason for hiding this comment

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

s/Algorithm/ViewAlgorithm/?

ast/ddl.go Outdated

// Security is VIEW's SQL SECURITY characteristic
// See https://dev.mysql.com/doc/refman/5.7/en/create-view.html
type Security int
Copy link
Member

Choose a reason for hiding this comment

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

s/Security/ViewSecurity/

ast/ddl.go Outdated
SecurityInvoker
)

var SecurityType = map[Security]string {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

ast/ddl.go Outdated

// CheckOption is VIEW's WITH CHECK OPTION clause part
// See https://dev.mysql.com/doc/refman/5.7/en/view-check-option.html
type CheckOption int
Copy link
Member

Choose a reason for hiding this comment

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

s/CheckOption/ViewCheckOption/

ast/ddl.go Outdated
CheckOptionCascaded
)

var CheckOptionType = map[CheckOption]string {
Copy link
Member

Choose a reason for hiding this comment

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

ditto

return v.Leave(newNode)
}
n = newNode.(*CreateViewStmt)
node, ok := n.ViewName.Accept(v)
Copy link
Member

Choose a reason for hiding this comment

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

should we visit n.ddlNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently ddlNode is not able to visit, and it only contains a statement string.

Copy link
Member

Choose a reason for hiding this comment

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

got it.

parser_test.go Outdated
@@ -15,6 +15,7 @@ package parser

import (
"fmt"
"github.com/pingcap/parser/model"
Copy link
Contributor

Choose a reason for hiding this comment

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

move this line down out of the native packages

parser_test.go Outdated
c.Assert(v.Cols[0], Equals, model.CIStr{"a", "a"})
c.Assert(v.Cols[1], Equals, model.CIStr{"b", "b"})
c.Assert(v.Cols[2], Equals, model.CIStr{"c", "c"})
c.Assert(strings.TrimSpace(v.Select.Text()), Equals, "select c,d,e from t")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we TrimSpace here?
If there are redundant spaces, we need to change parser.y to extract the exact sql text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just done yet...

ast/ddl.go Outdated
return "DEFINER"
}

// ViewCheckOption is VIEW's WITH CHECK OPTION clause part
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a . at the end of this comment.
So as the other comment lines if they are complete sentences.

@AndrewDi
Copy link
Contributor Author

@XuHuaiyu @zz-jason PTAL

if $11 !=nil {
x.CheckOption = $11.(ast.ViewCheckOption)
endOffset := parser.startOffset(&yyS[yypt])
selStmt.SetText(strings.TrimSpace(parser.src[startOffset:endOffset]))
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested it locally. Seems we do not need this strings.TrimSpace.

Copy link
Contributor Author

@AndrewDi AndrewDi Nov 30, 2018

Choose a reason for hiding this comment

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

If I remove strings.TrimSpace, extra space would make test fail.

FAIL: parser_test.go:2220: testParserSuite.TestView

parser_test.go:2265:
    c.Assert(v.Select.Text(), Equals, "select c,d,e from t")
... obtained string = "" +
...     "select c,d,e from t \n" +
...     "                  "
... expected string = "select c,d,e from t"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can use sql string for test:

src := `CREATE OR REPLACE ALGORITHM = UNDEFINED DEFINER = root@localhost
                  SQL SECURITY DEFINER
			      VIEW V(a,b,c) AS select c,d,e from t 
                  WITH CASCADED CHECK OPTION;`

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 3, 2018

LGTM
PTAL @zz-jason

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT2 LGT2 label Dec 3, 2018
@XuHuaiyu XuHuaiyu merged commit 2890715 into pingcap:master Dec 3, 2018
@AndrewDi AndrewDi deleted the extract_create_view_params branch December 5, 2018 13:01
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants