impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Date Fri, 03 Nov 2017 16:33:03 GMT
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447
)

Change subject: IMPALA-2181: Add query option levels for display
......................................................................


Patch Set 5: Code-Review-1

(3 comments)

I took a quick look through. I'm uncomfortable with this change as it stands, because I feel
it abuses the Thrift structures too much. Specifically, keeping the map of query option levels
in TQueryOptions seems wrong: you're really describing "Query Option Metadata" (which might
include name, description/help, and level), rather than the query options themselves. And,
for sending this stuff to impala-shell, abusing the 'description' field seems wrong. (In fact,
impala-shell should actually show useful help for these options. Things like "MT_DOP" are
completely opaque.

Implementation wise, I think we also need to worry about what happens for the HiveServer2
side of things. Specifically, we respond to "SET" queries over HS2 at https://github.com/apache/incubator-impala/blob/1803b403e38b3f952afc4ff3fd3e5f4d14c088f8/be/src/service/client-request-state.cc#L194.
I think we want to stay away from adding information into the Beeswax API that isn't also
exposed in HS2. One option is to let "SET ALL" be meaningful as a query for HS2, though I
don't quite know what that means in terms of the schema returned: how would you divide the
regular/advanced/deprecated options over that API?

http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift@65
PS5, Line 65: // Levels to use when displaying query options from Impala shell
Let's leave a pointer here indicating where the mapping from the options in TQueryOptions
to these levels is defined.


http://gerrit.cloudera.org:8080/#/c/8447/5/common/thrift/ImpalaInternalService.thrift@297
PS5, Line 297:   // Map for query option name to option level mapping. Populated by ImpalaServer
             :   // on startup
             :   61: required map<string, TQueryOptionLevel> query_option_levels;
This worries me a bit, for a couple of reasons.

First, 'required' is forever. (See https://diwakergupta.github.io/thrift-missing-guide/#_defining_structs.)
I think it means that a Thrift client will refuse to serialize a struct if this isn't set.
For some uses, like TClientRequest, it makes no sense for us to carry this around. It's possible
it works out because it can be set to the empty map often, but it's still odd.

Second, this isn't a query option. It's certainly describing query options, but it's very
very different from, say, "mem_limit", which applies to the query.


http://gerrit.cloudera.org:8080/#/c/8447/5/shell/impala_client.py
File shell/impala_client.py:

http://gerrit.cloudera.org:8080/#/c/8447/5/shell/impala_client.py@100
PS5, Line 100:   def get_query_option_level_from_description(self, description):
Have we decided we can't change ConfigVariable to include a level? It seems like this is very
much not a description, and we're abusing it.

/** Represents a Hadoop-style configuration variable. */
struct ConfigVariable {
  1: string key,
  2: string value,
  3: string description
}



-- 
To view, visit http://gerrit.cloudera.org:8080/8447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Nov 2017 16:33:03 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message