GroupBy: Validation of output names, and a gross hack for v1 subqueries.#3686
Merged
nishantmonu51 merged 1 commit intoapache:masterfrom Nov 29, 2016
Merged
GroupBy: Validation of output names, and a gross hack for v1 subqueries.#3686nishantmonu51 merged 1 commit intoapache:masterfrom
nishantmonu51 merged 1 commit intoapache:masterfrom
Conversation
Contributor
Author
|
Closing for now since this will conflict with #3685. |
gianm
force-pushed
the
groupby-validation
branch
from
33eab40 to
8048b9b
Compare
gianm
changed the title
Contributor
Author
|
Don't merge until #3685 is merged; this one is rebased off that. |
gianm
force-pushed
the
groupby-validation
branch
from
8048b9b to
0791c49
Compare
Contributor
|
@gianm this now has conflicts |
Contributor
|
👍 but FWIW, groupby v1 is dead to me |
leventov
reviewed
Nov 14, 2016
Member
There was a problem hiding this comment.
A small thing: toArray(new T[0]) is a better default choice now, see https://shipilev.net/blog/2016/arrays-wisdom-ancients/#_conclusion, "Bottom line" section.
Contributor
Author
There was a problem hiding this comment.
Oh, that's kind of surprising and good to know.
gianm
force-pushed
the
groupby-validation
branch
from
0791c49 to
0841689
Compare
Contributor
Author
|
Rebased |
v1 subqueries try to use aggregators to "transfer" values from the inner results to an incremental index, but aggregators can't transfer all kinds of values (strings are a common one). This is a workaround that selectively ignores what the outer aggregators ask for and instead assumes that we know best. These are in the same commit because the name validation changed the kinds of errors that were thrown by v1 subqueries.
gianm
force-pushed
the
groupby-validation
branch
from
0841689 to
6bff8aa
Compare
Member
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
few comments for log messages, otherwise seems good.
|
|
||
| for (AggregatorFactory aggregator : aggregators) { | ||
| if (!outputNames.add(aggregator.getName())) { | ||
| throw new IAE("Duplicate output name[%s]", aggregator.getName()); |
Member
There was a problem hiding this comment.
change log message to "Duplicate Aggregator name[%s]"
Contributor
Author
There was a problem hiding this comment.
I think that message would be misleading - the duplicate name might be a dimension and aggregator with the same name, not two aggregators.
|
|
||
| for (PostAggregator postAggregator : postAggregators) { | ||
| if (!outputNames.add(postAggregator.getName())) { | ||
| throw new IAE("Duplicate output name[%s]", postAggregator.getName()); |
Contributor
Author
There was a problem hiding this comment.
Similar comment to above
Contributor
|
@nishantmonu51 any more comments? |
dgolitsyn
pushed a commit
to metamx/druid
that referenced
this pull request
Feb 14, 2017
…es. (apache#3686) v1 subqueries try to use aggregators to "transfer" values from the inner results to an incremental index, but aggregators can't transfer all kinds of values (strings are a common one). This is a workaround that selectively ignores what the outer aggregators ask for and instead assumes that we know best. These are in the same commit because the name validation changed the kinds of errors that were thrown by v1 subqueries.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Prevents having dimensions, aggregators, or postaggregators with the same name as each other. In the past they would have clobbered each other, which would lead to unexpected query results.