-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add implementation for RunDQL for v25 #9355
Conversation
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.
Pull Request Overview
This PR implements and tests a new DQL query execution flow for v25, including the new RunDQL RPC and related parsing improvements.
- Replaces legacy ParseMutation calls with ParseDQL across tests and command handlers
- Introduces a new RunDQL method in edgraph/query.go with proper authentication and namespace resolution
- Adjusts lexer state handling in dql/state.go for better differentiation between mutation and query parsing
- Updates namespace-related tests to verify expected results and error messages
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
dql/dql_test.go | Adds tests for ParseDQL covering various valid and error scenarios |
edgraph/query.go | Implements RunDQL with guardian authentication and namespace lookup |
dql/dql.go & dql/parser_mutation.go | Migrates legacy ParseMutation to ParseDQL for unified parsing logic |
dql/state.go | Removes deprecated lexIdentifyBlock and introduces lexIdentifyMutationOrQuery |
edgraph/namespace_test.go | Updates tests to use RunDQL and check proper namespace error responses |
dgraph/cmd/alpha/http.go | Replaces ParseMutation with ParseDQL for HTTP mutation handling |
dql/upsert_test.go | Updates tests to verify behavior of new ParseDQL function |
edgraph/alter.go, edgraph/namespace.go | Adds/update SPDX headers and minor formatting changes |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (4)
dql/state.go:334
- [nitpick] Consider updating the accompanying comment to explain the purpose of lexIdentifyMutationOrQuery, which would improve code readability and maintainability.
return lexIdentifyMutationOrQuery
dgraph/cmd/alpha/http.go:405
- Verify that switching from ParseMutation to ParseDQL in the HTTP mutationHandler does not regress legacy handling of N-Quads, and ensure corresponding tests fully cover this updated behavior.
req, err = dql.ParseDQL(string(body))
edgraph/namespace_test.go:75
- [nitpick] Ensure that the error message format for missing namespaces remains consistent with the error returned by getNamespaceIDFromName to facilitate reliable test matching.
require.ErrorContains(t, err, "namespace \"ns2-new\" not found")
edgraph/query.go:32
- [nitpick] Consider refactoring the custom unauthorized error message in RunDQL into a defined constant to minimize duplication and simplify future modifications.
s := status.Convert(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.
Pull Request Overview
This PR introduces the RunDQL implementation for v25 and refactors the DQL parsing logic. Key changes include:
- Adding a new RunDQL API method in edgraph/query.go that performs guardian authorization and invokes the updated DQL parser.
- Refactoring the DQL package to consolidate ParseDQL in place of the deprecated ParseMutation functions and adjusting related tests.
- Updating the lexer state functions in dql/state.go along with miscellaneous updates in test and utility files.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
dql/dql_test.go | Added tests to verify ParseDQL functionality across various DQL query and mutation scenarios. |
edgraph/query.go | Introduced the RunDQL method with updated authorization and namespace resolution logic. |
dql/dql.go | Replaced ParseMutation with ParseDQL and updated the token handling logic accordingly. |
dql/state.go | Removed deprecated lexIdentifyBlock/lexNameBlock functions and introduced lexIdentifyMutationOrQuery. |
edgraph/alter.go & edgraph/namespace.go | Updated file headers and minor formatting changes. |
edgraph/namespace_test.go | Updated tests to use RunDQL and verify consistent error messages for missing namespaces. |
dgraph/cmd/alpha/http.go | Updated the mutation handler to call ParseDQL instead of the older ParseMutation. |
dql/upsert_test.go & dql/parser_test.go | Updated tests to reflect changes in the parsing logic and error messages. |
dql/parser_mutation.go | Changed lexer initialization from lexIdentifyBlock to lexTopLevel. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (3)
dql/state.go:334
- [nitpick] Consider renaming 'lexIdentifyMutationOrQuery' to a more descriptive name (e.g., 'lexDetermineOperation') to better clarify its purpose.
return lexIdentifyMutationOrQuery
edgraph/namespace_test.go:75
- [nitpick] Ensure that error messages for missing namespaces are consistently formatted across methods, including uniform quoting of namespace names.
require.ErrorContains(t, err, "namespace \"ns2-new\" not found")
dql/parser_mutation.go:18
- Replacing 'lexIdentifyBlock' with 'lexTopLevel' may introduce edge cases in mutation parsing; please verify that all mutation queries are correctly handled.
lexer.Run(lexTopLevel)
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.
Pull Request Overview
This PR implements RunDQL for v25 and updates DQL parsing and related tests. Key changes include:
- Addition of new unit tests for ParseDQL in dql/dql_test.go.
- Implementation of the RunDQL method in edgraph/query.go using the new ParseDQL.
- Updates to the DQL parsing logic and test expectations across multiple files.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
dql/dql_test.go | New test cases for validating ParseDQL functionality. |
edgraph/query.go | Added RunDQL implementation with guardian authorization and DQL parsing integration. |
dql/dql.go | Introduces the new ParseDQL function using lexTopLevel for consistent parsing. |
dql/state.go | Updates state machine functions to distinguish between mutations and queries. |
edgraph/namespace_test.go | Adjusted tests to use RunDQL and updated error message expectations for namespace operations. |
dgraph/cmd/alpha/http_test.go | Updated expected error message substrings for charset and request parsing scenarios. |
dgraph/cmd/alpha/http.go | Replaced use of ParseMutation with ParseDQL when handling RDF requests. |
dql/parser_test.go | Updated expected error substrings in mutation parsing tests. |
dql/parser_mutation.go | Changed lexer state function call from lexIdentifyBlock to lexTopLevel. |
dql/upsert_test.go | Modified tests to use ParseDQL and updated expected error messages accordingly. |
edgraph/namespace.go, alter.go | Added/upated SPDX headers. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
dql/dql.go:2
- The file header in dql/dql.go is inconsistent with other files which include the © symbol. Update the header to include © for consistency.
* SPDX-FileCopyrightText: Hypermode Inc. <[email protected]>
No description provided.