impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3548: Prune runtime filters based on query options in the FE
Date Wed, 06 Sep 2017 18:45:07 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3548: Prune runtime filters based on query options in the FE
......................................................................


Patch Set 4:

> > > > (1 comment)
 > > >
 > > > The tests in this change use only 3 query options. Adding a new
 > > > section to the .test files to support only these 3 options
 > > wouldn't
 > > > be too much work.
 > > >
 > > > On the other hand, adding support for all the query options
 > that
 > > > Impala supports would be a lot harder. Probably we would have
 > to
 > > > implement that using some Java reflection trickery.
 > >
 > > I don't think you need to use Java reflection. The generated Java
 > > class for TQueryOptions has a number of helper functions to
 > search
 > > and set a field by name. So, for instance the QUERY_OPTIONS
 > section
 > > could have key=value pairs that correspond to query options. Then
 > > we could write a small function that parses the key value pairs
 > and
 > > uses the helper functions to check for valid query options and
 > set
 > > the values. Do you want to give it a try? Thanks
 > 
 > Thanks. I'm still confused though how to implement setting enum
 > query options without reflection. e.g.:
 > ---- QUERYOPTIONS
 > COMPRESSION_CODEC=GZIP
 > 
 > Here we have to know that the type of TQueryOptions.compression_codec
 > is org.apache.impala.thrift.THdfsCompression, otherwise we cannot
 > parse "GZIP". Am I missing something?

If you detect that this query option refers to a compression_codec, the only thing you need
to do is map the string literal "GZIP" to an instance of HdfsCompression (the latter is just
an enum). From that you can get the THdfsCompression object that you pass in the setFieldValue()
method. You don't need to handle all the query options in one go. You can add the infrastructure
needed, i.e. parsing the key=value pairs, validating the key part and then simply add the
logic to handle the options we're interesting in for this patch. Once the infrastructure is
in place, then adding support for other options should be quite easy, but you don't need to
do everything.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0f0b200e02442edcad8df3979f652d66c6e52eb
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: No

Mime
View raw message