impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Kaszab (Code Review)" <ger...@cloudera.org>
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. ( 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 <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: Wed, 22 Nov 2017 05:16:52 +0000
Gerrit-HasComments: Yes

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