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

Parser refactor #2121

Merged
merged 1 commit into from
Apr 9, 2015
Merged

Parser refactor #2121

merged 1 commit into from
Apr 9, 2015

Conversation

dgnorton
Copy link
Contributor

The big change in this PR is how identifiers are scanned / parsed. The scanner used to treat "db"."rp".cpu as one identifer. It would be stored in a single string in the AST somewhere. The scanner API exposed SplitIdent and it was up to clients consuming the AST to split identifiers into segments, strip quotes from the segments, and figure out what each segment was (db?, rp?, measurement?). This PR simplifies things for client code by splitting identifiers and stripping quotes at the earliest stage (scanning) and changing the parser to store them in appropriate fields (db, rp, etc.) in the AST.

  • Change Scanner.scanIdent() to scan "db"."rp".cpu as separate tokens: IDENT, DOT, IDENT, DOT, IDENT instead of scanning it as a single IDENT
  • Add Parser.parseSegmentedIdents() to parse segmented (DOT separated) identifiers like "db"."rp".cpu and return them as an []string
  • Change AST types that held segmented identifiers in a string to have individual fields. Note that type VarRef still, for now, holds a single string. This may need to be changed.
  • Add type HasDefaultDatabase interface to ast.go. Types like type CreateContinuousQueryStatement that have a default database should implement this interface.
  • Change parsing functions that work with segmented identifiers to use the new parseSegmentedIdents() function and store segments in the appropriate AST fields.
  • Change QuoteIdent function to only quote segments that must be quoted.
  • Add IdentNeedsQuotes func that returns true if an identifier would require quotes
  • Change Server.ExecuteQuery to check for a default database passed by the caller. If none was provided, see if the statement implements the HasDefaultDB interface and get the default database from there. This fixes a bug in statement normalization.
  • Remove uses of influxql.SplitIdent function outside of the parser
  • Change Server.expandSources to always return expanded sources in sorted order
  • Change ErrDatabaseNotFound and ErrRetentionPolicyNotFound to be functions that report the name of the "not found" object and the error locust

@@ -177,7 +178,7 @@ func write(t *testing.T, node *Node, data string) {

// query executes the given query against all nodes in the cluster, and verifies no errors occured, and
// ensures the returned data is as expected
func query(t *testing.T, nodes Cluster, urlDb, query, expected string) (string, bool) {
func query(t *testing.T, nodes Cluster, urlDb, query, expected, expectPattern string) (string, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making this a vargs? Then all the callers won't need to be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should have two versions of this function query and queryRegex. You could factor out the common code, but two functions might be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would we also need two versions of queryAndWait then? I wasn't crazy about adding the extra parameter but it seemed like the simplest solution. If we factor into two funcs, every caller has to do the if expected != "" check. No strong preference here...just don't want to add complication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, wasn't sure about this. We can come back it.

@otoolep
Copy link
Contributor

otoolep commented Mar 31, 2015

Took a first pass, will re-review again soon. I like the introduction of regex matching into the test suite, very handy. I'm not keen on the new HasDefaultDB interface though, it feels a bit awkward, though I am not in the code so it may be required. I wonder if there is another way, perhaps every statement just have this call, period.

@dgnorton dgnorton force-pushed the fix-2015 branch 2 times, most recently from 3a8bebc to 8e59277 Compare April 8, 2015 16:39
@dgnorton dgnorton changed the title (WIP) Parser refactor Parser refactor Apr 8, 2015
queryDb string // If set, is used as the "db" query param.
queryOne bool // If set, only 1 node is queried.
expected string // If 'query' is equal to the blank string, this is ignored.
expectPattern string // Regexp alternative to expected field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: document that this is ignored if expected is non-blank.

@otoolep
Copy link
Contributor

otoolep commented Apr 8, 2015

OK, I took a look at this. It's obviously a very large change but most of it is adding extra variables to functions for example. The tests haven't changed that much, so the green build gives me confidence too.


vr := &VarRef{Val: strings.Join(segments, ".")}

if len(segments) > 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, if we have more than 2 segments here, it's a problem? Normally we can have up to 3 segments. Which segment is not expected to be present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That's probably a mistake...not sure why I did that. Will review.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, cool. Might indicate that a unit test is needed to check a specific code path.

@dgnorton dgnorton force-pushed the fix-2015 branch 2 times, most recently from a52bb5e to 41b4a0f Compare April 9, 2015 17:20
toddboom added a commit that referenced this pull request Apr 9, 2015
@toddboom toddboom merged commit ff15388 into master Apr 9, 2015
@toddboom toddboom deleted the fix-2015 branch April 9, 2015 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants