Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 )
Change subject: IMPALA-2181: Add query option levels for display ...................................................................... Patch Set 15: (17 comments) http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@10 PS14, Line 10: diplayed displayed http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@12 PS14, Line 12: called SET ALL shows all the options grouped by their option levels. Mention that there are also server-side versions of these commands? http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/impala-server.cc@1236 PS14, Line 1236: else { Nit: } else { http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@45 PS15, Line 45: QUERY_OPT_FN(disable_cached_reads, DISABLE_CACHED_READS, TQueryOptionLevel::REGULAR)\ Looks like disable_cached_reads is deprecated: see IMPALA-2963 . I missed that on an earlier pass http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@89 PS15, Line 89: QUERY_OPT_FN(enable_expr_rewrites, ENABLE_EXPR_REWRITES, TQueryOptionLevel::REGULAR)\ I think enable_expr_rewrites should be advanced. It's only disabled as a workaround to planner bugs. http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@91 PS15, Line 91: QUERY_OPT_FN(parquet_dictionary_filtering, PARQUET_DICTIONARY_FILTERING, TQueryOptionLevel::REGULAR)\ parquet_dictionary_filtering should probably be advanced, there's typically not much reason to disable it unless working around a bug. http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@93 PS15, Line 93: QUERY_OPT_FN(parquet_read_statistics, PARQUET_READ_STATISTICS, TQueryOptionLevel::REGULAR)\ Same for parquet_read_statistics. http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.h@39 PS14, Line 39: QUERY_OPT_FN(abort_on_default_limit_exceeded, ABORT_ON_DEFAULT_LIMIT_EXCEEDED, TQueryOptionLevel::DEPRECATED)\ nit: can we wrap these to 90 chars? http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8447/14/be/src/service/query-options.cc@41 PS14, Line 41: using namespace beeswax; nit: probably best to individually import the names so it's easier to figure out what got imported. http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/ImpalaService.thrift@294 PS14, Line 294: typedef map<string, beeswax.TQueryOptionLevel> TQueryOptionLevels Does this need to be a typedef here? I don't see any references in any thrift files. Maybe we can just use the map type directly where its needed in the backend http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift File common/thrift/beeswax.thrift: http://gerrit.cloudera.org:8080/#/c/8447/14/common/thrift/beeswax.thrift@111 PS14, Line 111: 4: optional TQueryOptionLevel level Comment that this was added for impala-shell? http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java File fe/src/main/java/org/apache/impala/analysis/SetStmt.java: http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@55 PS14, Line 55: if (isSetAll_) { Nit: could be one line if (isSetAll_) return ...; http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@76 PS14, Line 76: request.setIs_set_all(true); nit: could be one line http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_client.py@97 PS14, Line 97: self.query_option_levels[option.key.upper()] = option.level Does this still work if we're talking to an older Impala server that doesn't return the level? Update: looks like it doesn't: Traceback (most recent call last): File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 15 27, in <module> shell = ImpalaShell(options, query_options) File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 20 5, in __init__ self.do_connect(options.impalad) File "/home/tarmstrong/Impala/incubator-impala/shell/impala_shell.py", line 73 1, in do_connect self.imp_client.build_default_query_options_dict() File "/home/tarmstrong/Impala/incubator-impala/shell/impala_client.py", line 9 7, in build_default_query_options_dict self.query_option_levels[option.key.upper()] = option.level AttributeError: ConfigVariable instance has no attribute 'level' http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/8447/14/shell/impala_shell.py@82 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 the levels defined in TQueryOptionLevel" http://gerrit.cloudera.org:8080/#/c/8447/14/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/8447/14/tests/hs2/test_hs2.py@53 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 constructor - see shell/build/impala-shell-2.11.0-SNAPSHOT/gen-py/TCLIService/ttypes.py The existing tests follow this pattern but it seems like a weird pattern. http://gerrit.cloudera.org:8080/#/c/8447/14/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/8447/14/tests/shell/test_shell_interactive.py@359 PS14, Line 359: IMAPA IMPALA -- 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: 15 Gerrit-Owner: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 21 Nov 2017 01:06:42 +0000 Gerrit-HasComments: Yes
