Group by time in the GROUP BY clause RTS-1689 #1600
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
andytill
changed the title
|
Getting some crashes |
This was referenced Jan 24, 2017
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
Works on my machine ;) Do you have the |
paegun
reviewed
Jan 24, 2017
src/riak_kv_qry_compiler.erl
Outdated
|
|
||
| -spec make_group_by_time_fn(module(), group_time_fn()) -> function(). | ||
| make_group_by_time_fn(Mod, {time_fn, {identifier,FieldName},{integer,GroupSize}}) -> | ||
| Pos = Mod:get_field_position([FieldName]), |
There was a problem hiding this comment.
I don't see any handling in tests or in code for specification of a field that does not exist. An error should be raised if the time function field parameter does not exist in the table.
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_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
✅ MAKE_DEPS
✅ MAKE_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
|
Looks like we still have some test/dialyzer failures... |
|
@javajolt it relies on the |
|
got it. sorry for missing that. |
hmmr
reviewed
Feb 21, 2017
src/riak_kv_qry_compiler.erl
Outdated
| group_by_column_does_not_exist_error(Mod, FieldName) -> | ||
| ?DDL{table = TableName} = Mod:get_ddl(), | ||
| Msg = iolist_to_binary(io_lib:format( | ||
| "Error in group by clause, column '~ts' does not exist in table table ~ts", [FieldName, TableName])), |
There was a problem hiding this comment.
For uniformity's sake, can you -define the body of the message in riak_kv_ts_error_msgs.hrl?
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
hmmr
reviewed
Feb 21, 2017
src/riak_kv_ts_error_msgs.hrl
Outdated
| -define( | ||
| E_MISSING_COL_IN_GROUP_BY(FieldName, TableName), | ||
| iolist_to_binary(io_lib:format( | ||
| "Error in group by clause, column '~ts' does not exist in table table ~ts", [FieldName, TableName])) |
There was a problem hiding this comment.
I can shorten this to "Column '~s' in GROUP BY clause does not exist in table '~ts'". (Or at least, delete one word "table" that appears in duplicate there.)
There seems to be an issue with build step **make_test,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
⛔ MAKE_DIALYZER
|
|
+1 |
1 similar comment
|
+1 |
added 9 commits
February 22, 2017 16:52
hazen
force-pushed
the
at-group-by-time
branch
from
403d04a
to
c534bc7
Compare
|
rebased this baby, too. will retry once QL has merged |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 1 of 2 Code reviews from organization basho
|
|
+1 |
1 similar comment
|
+1 |
|
Merging and closing this pr |
|
Successfully merged basho/riak_kv/pulls/1600 (6ed5838 on to riak_ts-develop) ---
:sha: c2d7ce5834d6ef8d1396248acb56f1b4670b0a6e
:merged: true
:message: Pull Request successfully merged
|
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.
Allowing grouping by time in the
GROUP BYclause.RFC: basho/rfc#50
riak_ql: basho/riak_ql#164
riak_kv: #1600
riak_test: basho/riak_test#1266