You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2017/08/18 17:37:18 UTC
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Bikramjeet Vig has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7721
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
IMPALA-5784 : Separate planner and user set query options in profile
This separation will help the user better understand the query
runtime profile.
Testing:
Modified an existing test case.
Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
---
M be/src/service/client-request-state.cc
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_observability.py
3 files changed, 14 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/7721/1
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
PS3, Line 149: manually
Hm. This word may be misleading, query options may have been set when assigned to an admission control pool, i.e. the per-pool defaults. Also there are server-wide defaults that can be set by the gflag -default_query_options, and those would show up here too.
'manually or automatically set' is more precise, but verbose. Lets see if Dan or Balasz have suggestions.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
> > > > > (1 comment)
> > > > Relying to Dan and Balasz:
> > > >
> > > > It's actually hard to determine what gets set where using the
> > > > QueryOptions map thing we have right now, mostly because of
> how
> > > > inconsistent we are with default values. E.g. what if a
> > property
> > > is
> > > > set in the session, then the planner sets it as well, but
> > happens
> > > > to set it to the default value. Right now we can't even
> > identify
> > > > that as a non-default query option.
> > > >
> > >
> > > I don't follow this. We're defining "non-default" query option
> to
> > > mean everything that doesn't match the static query option
> value,
> > > right?
> > >
> > > If so, why can't we just compare the query option values after
> > > planning and to the values before planning, and print any
> > > differences as "planner set" (or whatever name). And then
> > compare
> > > the query options before planning with the static defaults,
> > remove
> > > anything we've already printed as "planner set" and print those
> > > remaining differences as "manually set".
> > >
> > > or are you saying that doing that is tricky due to the
> > > datastructures we currently have?
> >
> > Yes, I'm saying the data structures we have right now don't lend
> > themselves to what you're describing. Its certainly possible, but
> > it'd be awkward given how it currently works and I think it'd be
> > kind of a pain to test and make sure we don't break different
> cases
> > (e.g. values set in the session/query x modified or not by the
> > planner x with/without a static default). If you think it's worth
> > it in terms of usability, then Bikram can keep exploring that
> path.
>
> The code in query-options.cc/h is pretty hairy, e.g.
> https://github.com/apache/incubator-impala/blob/master/be/src/service/query-options.cc#L82
Okay, let's go with this for now then, but please see Jeszy's naming suggestion.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1: Code-Review+1
lgtm, let's give others a chance to look too. we have to wait for IMPALA-5602 to go in anyway.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1140/
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
IMPALA-5784: Separate planner and user set query options in profile
This separation will help the user better understand the query
runtime profile.
Testing:
Modified an existing test case.
Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Reviewed-on: http://gerrit.cloudera.org:8080/7721
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/service/client-request-state.cc
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_observability.py
3 files changed, 13 insertions(+), 11 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Matthew Jacobs: Looks good to me, but someone else must approve
Dan Hecht: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
Line 151: summary_profile_.AddInfoString("Query Options (non default, after planning)",
> I'd change 'before planning' and 'after planning' to 'manually set' and 'pl
Done
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3//COMMIT_MSG
Commit Message:
PS3, Line 7:
nit: extra space
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change.
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
PS3, Line 149: manually
> Seems okay. Or slight tweak:
Done
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
> (1 comment)
Relying to Dan and Balasz:
It's actually hard to determine what gets set where using the QueryOptions map thing we have right now, mostly because of how inconsistent we are with default values. E.g. what if a property is set in the session, then the planner sets it as well, but happens to set it to the default value. Right now we can't even identify that as a non-default query option.
Obviously anything is possible with more changes, but we'd probably need to change how the query options map works so that they have a standard scheme for providing default values and are properly support composition and diff'ing, e.g. the planner sets the options = the new map - the old map.
Not bad to think about going down that path, but it'll be a bigger change and not for Impala 2.10. I think the safer thing is to print the maps more honestly (i.e. less special interpretation) and settle on some names for these fields that we're happy with.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7721
to look at the new patch set (#2).
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
IMPALA-5784 : Separate planner and user set query options in profile
This separation will help the user better understand the query
runtime profile.
Testing:
Modified an existing test case.
Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
---
M be/src/service/client-request-state.cc
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_observability.py
3 files changed, 14 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/7721/2
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
Line 151: summary_profile_.AddInfoString("Query Options (non default, after planning)",
this is probably okay, but i wonder if it'd be clearer if we instead either:
A) just have mutually exclusive lists where the options set by the impala are in one list and options set by the user/session are in the other list.
B) have a single list where options set by Impala are marked with an asterisk or something.
if that's much complexity, maybe not worth it. also interested to hear what others think about which is easier to read.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
PS3, Line 149: manually
> Seems okay. Or slight tweak:
lgtm
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
PS3, Line 149: manually
> That's a good point. Also, referring to these as "non-default" can be confu
Yeah true. How about
Query Options (set by configuration)
Query Options (including planner set)
?
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
PS3, Line 149: manually
> Hm. This word may be misleading, query options may have been set when assig
That's a good point. Also, referring to these as "non-default" can be confusing given that the startup flag is called --default_query_options, and so you might expect this list to include the values that are different from those. I don't have a great suggestion right now though. Ideas?
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7721
to look at the new patch set (#4).
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
IMPALA-5784: Separate planner and user set query options in profile
This separation will help the user better understand the query
runtime profile.
Testing:
Modified an existing test case.
Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
---
M be/src/service/client-request-state.cc
M tests/custom_cluster/test_admission_controller.py
M tests/query_test/test_observability.py
3 files changed, 13 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/7721/4
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
Initial Patch, need to make sure to change a test case added in IMPALA-5602 (after its merged) that will break due to this change
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
> > > > (1 comment)
> > > Relying to Dan and Balasz:
> > >
> > > It's actually hard to determine what gets set where using the
> > > QueryOptions map thing we have right now, mostly because of how
> > > inconsistent we are with default values. E.g. what if a
> property
> > is
> > > set in the session, then the planner sets it as well, but
> happens
> > > to set it to the default value. Right now we can't even
> identify
> > > that as a non-default query option.
> > >
> >
> > I don't follow this. We're defining "non-default" query option to
> > mean everything that doesn't match the static query option value,
> > right?
> >
> > If so, why can't we just compare the query option values after
> > planning and to the values before planning, and print any
> > differences as "planner set" (or whatever name). And then
> compare
> > the query options before planning with the static defaults,
> remove
> > anything we've already printed as "planner set" and print those
> > remaining differences as "manually set".
> >
> > or are you saying that doing that is tricky due to the
> > datastructures we currently have?
>
> Yes, I'm saying the data structures we have right now don't lend
> themselves to what you're describing. Its certainly possible, but
> it'd be awkward given how it currently works and I think it'd be
> kind of a pain to test and make sure we don't break different cases
> (e.g. values set in the session/query x modified or not by the
> planner x with/without a static default). If you think it's worth
> it in terms of usability, then Bikram can keep exploring that path.
The code in query-options.cc/h is pretty hairy, e.g. https://github.com/apache/incubator-impala/blob/master/be/src/service/query-options.cc#L82
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/3/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
PS3, Line 149: manually
> Yeah true. How about
Seems okay. Or slight tweak:
Query Options (set by configuration)
Query Options (set by configuration and planner)
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784: Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784: Separate planner and user set query options in profile
......................................................................
Patch Set 4: Code-Review+1
Great, thanks!
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
> > (1 comment)
> Relying to Dan and Balasz:
>
> It's actually hard to determine what gets set where using the
> QueryOptions map thing we have right now, mostly because of how
> inconsistent we are with default values. E.g. what if a property is
> set in the session, then the planner sets it as well, but happens
> to set it to the default value. Right now we can't even identify
> that as a non-default query option.
>
I don't follow this. We're defining "non-default" query option to mean everything that doesn't match the static query option value, right?
If so, why can't we just compare the query option values after planning and to the values before planning, and print any differences as "planner set" (or whatever name). And then compare the query options before planning with the static defaults, remove anything we've already printed as "planner set" and print those remaining differences as "manually set".
or are you saying that doing that is tricky due to the datastructures we currently have?
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Balazs Jeszenszky (Code Review)" <ge...@cloudera.org>.
Balazs Jeszenszky has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7721/1/be/src/service/client-request-state.cc
File be/src/service/client-request-state.cc:
Line 151: summary_profile_.AddInfoString("Query Options (non default, after planning)",
> this is probably okay, but i wonder if it'd be clearer if we instead either
I'd change 'before planning' and 'after planning' to 'manually set' and 'planner set', and make the lists mutually exclusive. If mutually exclusive isn't worth it, 'after planning' could be called 'including planner set'.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5784 : Separate planner and user set query options in profile
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-5784 : Separate planner and user set query options in profile
......................................................................
Patch Set 1:
> > > (1 comment)
> > Relying to Dan and Balasz:
> >
> > It's actually hard to determine what gets set where using the
> > QueryOptions map thing we have right now, mostly because of how
> > inconsistent we are with default values. E.g. what if a property
> is
> > set in the session, then the planner sets it as well, but happens
> > to set it to the default value. Right now we can't even identify
> > that as a non-default query option.
> >
>
> I don't follow this. We're defining "non-default" query option to
> mean everything that doesn't match the static query option value,
> right?
>
> If so, why can't we just compare the query option values after
> planning and to the values before planning, and print any
> differences as "planner set" (or whatever name). And then compare
> the query options before planning with the static defaults, remove
> anything we've already printed as "planner set" and print those
> remaining differences as "manually set".
>
> or are you saying that doing that is tricky due to the
> datastructures we currently have?
Yes, I'm saying the data structures we have right now don't lend themselves to what you're describing. Its certainly possible, but it'd be awkward given how it currently works and I think it'd be kind of a pain to test and make sure we don't break different cases (e.g. values set in the session/query x modified or not by the planner x with/without a static default). If you think it's worth it in terms of usability, then Bikram can keep exploring that path.
--
To view, visit http://gerrit.cloudera.org:8080/7721
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfc7832963fa0bd278a45c06a5a54e1bf40d8876
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Balazs Jeszenszky <je...@gmail.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: No