Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/8447 )
Change subject: IMPALA-2181: Add query option levels for display ...................................................................... Patch Set 17: (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: displaye > displayed Done http://gerrit.cloudera.org:8080/#/c/8447/14//COMMIT_MSG@12 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? Done 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: config.__set_level(TQueryOptionLevel::ADVANCED); > Nit: Done 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: TQueryOptionLevel::DEPRECATED)\ > Looks like disable_cached_reads is deprecated: see IMPALA-2963 . I missed t Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@89 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 Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@91 PS15, Line 91: QUERY_OPT_FN(runtime_filter_mode, RUNTIME_FILTER_MODE, TQueryOptionLevel::REGULAR)\ > parquet_dictionary_filtering should probably be advanced, there's typically Done http://gerrit.cloudera.org:8080/#/c/8447/15/be/src/service/query-options.h@93 PS15, Line 93: TQueryOptionLevel::ADVANCED)\ > Same for parquet_read_statistics. Done 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: // If the DCHECK is hit then handle the missing query option below and update > nit: can we wrap these to 90 chars? Done 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 strings; > nit: probably best to individually import the names so it's easier to figur Done 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: // 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 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: // For displaying purposes in Impala shell > Comment that this was added for impala-shell? Done 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_) return "SET ALL"; > Nit: could be one line Done http://gerrit.cloudera.org:8080/#/c/8447/14/fe/src/main/java/org/apache/impala/analysis/SetStmt.java@76 PS14, Line 76: } > nit: could be one line Done 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: # If connected to an Impala that predates IMPALA-2181 then the received options > Does this still work if we're talking to an older Impala server that doesn' Nice catch! Done 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 t Done 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 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. 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: IMPAL > IMPALA Done -- 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: 17 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 22 Nov 2017 05:16:52 +0000 Gerrit-HasComments: Yes
