impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Kaszab (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2181: Add query option levels for display
Date Wed, 22 Nov 2017 05:16:52 GMT
Gabor Kaszab has posted comments on this change. ( )

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

Patch Set 17:

Commit Message:
PS14, Line 10: displaye
> displayed
PS14, Line 12: command called SET ALL shows all the options grouped by their option levels.
> Mention that there are also server-side versions of these commands?
File be/src/service/
PS14, Line 1236:     config.__set_level(TQueryOptionLevel::ADVANCED);
> Nit: 
File be/src/service/query-options.h:
PS15, Line 45:       TQueryOptionLevel::DEPRECATED)\
> Looks like disable_cached_reads is deprecated: see IMPALA-2963 . I missed t
PS15, Line 89:   QUERY_OPT_FN(disable_streaming_preaggregations, DISABLE_STREAMING_PREAGGREGATIONS,\
> I think enable_expr_rewrites should be advanced. It's only disabled as a wo
PS15, Line 91:   QUERY_OPT_FN(runtime_filter_mode, RUNTIME_FILTER_MODE, TQueryOptionLevel::REGULAR)\
> parquet_dictionary_filtering should probably be advanced, there's typically
PS15, Line 93:       TQueryOptionLevel::ADVANCED)\
> Same for parquet_read_statistics.
File be/src/service/query-options.h:
PS14, Line 39: // If the DCHECK is hit then handle the missing query option below and update
> nit: can we wrap these to 90 chars?
File be/src/service/
PS14, Line 41: using namespace strings;
> nit: probably best to individually import the names so it's easier to figur
File common/thrift/ImpalaService.thrift:
PS14, Line 294: // TODO: Rename to reflect that this is for all DML.
> Does this need to be a typedef here? I don't see any references in any thri
sure. I'll move this to query-options.h
File common/thrift/beeswax.thrift:
PS14, Line 111:   // For displaying purposes in Impala shell
> Comment that this was added for impala-shell?
File fe/src/main/java/org/apache/impala/analysis/
PS14, Line 55:       if (isSetAll_) return "SET ALL";
> Nit: could be one line 
PS14, Line 76: }
> nit: could be one line
File shell/
PS14, Line 97:       # If connected to an Impala that predates IMPALA-2181 then the received
> Does this still work if we're talking to an older Impala server that doesn'
Nice catch!
File shell/
PS14, Line 82:   """These are the levels used when displaying query options.
> Can you add a pointer to the thrift definition. E.g. "These correspond to t
File tests/hs2/
PS14, Line 53:     fetch_results_req.operationHandle = execute_statement_resp.operationHandle
> Not a big deal but we could pass in these fields as keyword args to the con
This function is actually a copy paste: I moved it out from test_session_options_via_set so
that I can re-use it.
To be honest I'd leave it as it is now.
File tests/shell/
PS14, Line 359: IMPAL

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75720d0d454527e1a0ed19bb43cf9e4f018ce1d1
Gerrit-Change-Number: 8447
Gerrit-PatchSet: 17
Gerrit-Owner: Gabor Kaszab <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Csaba Ringhofer <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Laszlo Gaal <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Wed, 22 Nov 2017 05:16:52 +0000
Gerrit-HasComments: Yes

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