Added a delete statement to the SQL set #145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit includes: * lexer changes * parser changes * a test suite The SQL statement is fully validated by the parsers so there are no error conditions that are returned at the lexer/parser stage. Validation that the WHERE clause covers the key and the table exists will be done in the query sub-system at run time, obviously.
|
Thanks @gordonguthrie! |
|
.thumbs.yml config: |
|
Build Status: [b77a3b8] Looks good! 👍
|
added 2 commits
October 26, 2016 11:39
This was a convenience for test writing (use the same key twice) but doesn't reflect reality and no longer works with delete key helper fns being generated - it has therefore been removed.
|
Build Status: [3295d51] Looks like there's an issue with build step make_dialyzer ! ☁️
|
There seems to be an issue with build step **make_dialyzer** ! ☁️
|
There seems to be an issue with build step **make_dialyzer** ! ☁️
|
There seems to be an issue with build step **make_dialyzer** ! ☁️
|
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️
|
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️⛔ MERGE
⛔ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
added 2 commits
November 24, 2016 12:30
This commit includes: * lexer changes * parser changes * a test suite The SQL statement is fully validated by the parsers so there are no error conditions that are returned at the lexer/parser stage. Validation that the WHERE clause covers the key and the table exists will be done in the query sub-system at run time, obviously. In addition a new function has been added to the DDL helper functions and a new DDL helper utility module has been added to make extending that easier. Some minor clean up was done: * we used to use the same local/partition key in some tests for convenience these fail validation now and have been removed
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️⛔ MERGE
⛔ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️⛔ MERGE
⛔ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
added 2 commits
November 28, 2016 14:42
Conflicts: * src/riak_ql_ddl.erl * src/riak_ql_ddl_compiler.erl * src/riak_ql_lexer.xrl * src/riak_ql_parser.yrl
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN_DEPS_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
paegun
reviewed
Nov 30, 2016
| "LocalKey = ~p, " | ||
| "case riak_ql_ddl_util:is_valid_delete_where_clause(W) of " | ||
| "true -> riak_ql_ddl_util:make_delete_key(LocalKey, W); " | ||
| "{error, Errors} -> {false, Errors} " |
There was a problem hiding this comment.
why not {error, Errors} ? The corresponding case will be comparing {ok, _} | {false, _}, but {ok, _} | {error, _} is idiomatic afaik.
There was a problem hiding this comment.
This is a bug - fixed and error path tests added
hmmr
reviewed
Nov 30, 2016
|
|
||
| is_valid_delete_where_clause(Where) -> | ||
| case is_valid2(Where, ?EMPTY_ERRORS) of | ||
| ?EMPTY_ERRORS -> true; |
There was a problem hiding this comment.
How does the validation deal with queries with insufficient values? As I read the code, make_delete_key will return undefined for each key field not mentioned in 'WHERE', which would lead to deleting.. what?
There was a problem hiding this comment.
This is a bug fixed, and tests added
| {where, [ | ||
| {'>', <<"bish">>, {integer, 1}} | ||
| ]} | ||
| ]). |
There was a problem hiding this comment.
A failing test would be appropriate here, too.
There was a problem hiding this comment.
The failure tests you want properly belong in riak_ql_ddl_compiler.erl and have been added there
This is because they need a compiled DDL helper module
| {RevertOrderingFn, LineNo11} = build_revert_ordering_on_local_key_fn(DDL, LineNo10), | ||
| {MinDDLCapFn, LineNo12} = build_min_ddl_version_fn(DDL, LineNo11), | ||
| {MinDDLCapFn, LineNo12} = build_min_ddl_version_fn(DDL, LineNo11), | ||
| {DeleteKeyFn, LineNo11} = build_delete_key_fn(DDL, LineNo10, []), |
There was a problem hiding this comment.
The progressive use and assignments of LineNos seems to be broken (both DeleteKeyFn and RevertOrderingFn take LineNo10 as a parameter).
There was a problem hiding this comment.
You have caught the dirty secret the line numbers are only used in decompilation - but have fixed anyway ;-)
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️⛔ MERGE
⛔ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
The error handling path wasn't being addressed for DELETE This includes fixes for that and a new set of unit tests
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️⛔ MERGE
⛔ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
There seems to be an issue with build step **merge,make_clean_deps_compile,make_test,make_xref,make_dialyzer** ! ☁️⛔ MERGE
⛔ MAKE_CLEAN_DEPS_COMPILE
⛔ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
…ield_type -> external_field_type). removed trailing whitespace found in diff w/ upstream.
There seems to be an issue with build step **make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN_DEPS_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 1 of 2 Code reviews from organization basho
|
|
+1 |
1 similar comment
|
+1 |
|
thumbot retry |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit includes:
The SQL statement is fully validated by the parsers so there are no error conditions that
are returned at the lexer/parser stage.
Validation that the WHERE clause covers the key and the table exists will be
done in the query sub-system at run time, obviously.