[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2498/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 06:38:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3938/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 06:12:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 11: Code-Review+2 Fixed minor issue. Carry +2. -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 06:12:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. IMPALA-8317: Add support for list type flags in Impala shell config file This patch adds support for list type flags in Impala shell config file, i.e. those that use action="append", such as --var and --query_option. To make it less error-prone, this patch also updates the logic for bool flags in the config file to also look at the correct type from the argument parser instead of relying on whether or not the default values are set in impala_shell_config_defaults.py. Testing: - Added a new test for list type flags - Ran all shell E2E tests Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 --- M shell/impala_shell.py M shell/option_parser.py M tests/shell/good_impalarc M tests/shell/test_shell_commandline.py M tests/shell/test_shell_interactive.py 5 files changed, 59 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12781/11 -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 16: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3937/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 16 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 05:40:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. Patch Set 18: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3936/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 18 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 05:38:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 16 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 05:40:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. Patch Set 18: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 18 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 05:38:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 10: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/3930/ -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 05:14:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 ) Change subject: IMPALA-2990: timeout unresponsive queries in coordinator .. Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/12299/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12299/4//COMMIT_MSG@30 PS4, Line 30: thread pool instead of a single thread?) not too concerned about this -- average query concurrency is going to be in the hundreds, maximum, and looping over even a few thousand queries every few seconds is not going to be a tax. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.h@140 PS4, Line 140: If nit: lower case 'i' http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.cc@750 PS4, Line 750: if (exec_rpcs_complete_barrier_ == nullptr this is racy since exec_rpcs_complete_barrier_ is changed from nullptr to a valid pointer without any locking. Could we change this to not be a scoped_ptr but instead just initialized in the ctor to avoid this? http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@215 PS4, Line 215: DEFINE_int32(hung_backend_check_interval_s, 30, "The time, in seconds, to wait between " instead of introducing a new flag, can we just set this based on the configured kill timeout? eg we can just say that we always want to kill a query within 10% of the target kill timeout, and set this to 0.1*FLAGS_status_report_interval_ms. That way we don't have two flags that have some confusing interaction. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@398 PS4, Line 398: periodic status reporting is enabled unrelated to this patch, but why is this even an option? shouldn't the backends _always_ report status? Can we deprecate this configuration? http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2226 PS4, Line 2226: int64_t max_lag_ms = GetMaxReportRetryMs(); Instead of forcing that the coordinator and executors all agree on this configuration, would it be easy enough to plumb it through as part of the RPC that starts execution for a given query? if it's not trivial, i'm fine with this as is. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2227 PS4, Line 2227: LOG(INFO) << "Queries will be cancelled if a backend has not reported its status in " : << "more than " << max_lag_ms << "ms."; don't we already log all the flags at startup? I think it'd be better to avoid an additional INFO if it's already logged elsewhere. Maybe change to VLOG(1)? http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2237 PS4, Line 2237: Offer I noticed that 'Offer' seems to call to BlockingPut under the covers, which means this might have to acquire a lock and wait. It's unlkely since cancellation_thread_pool_ has a very long queue length, but still gives me the heebie-jeebies, because this lambda is executed while holding a shard lock in ShardedQueryMap. Instead, maybe worth putting a vector> to_cancel outside the DoFuncForAllEntries call, and pushing the request_state onto that vector here, and then putting it onto the cancellation pool following that. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2288 PS4, Line 2288: // The max time that each retry takes is FLAGS_backend_client_rpc_timeout_ms. : // The wait time before a retry is: : // ( + 1) * FLAGS_status_report_interval_ms : // so max possible wait time is: : // (1 + 2 + ... + ) * FLAGS_status_report_interval_ms : // and 1 + 2 + ... + n = (n * (n + 1)) / 2 are these flags documented and user-settable? Is it too late to change the flag to just be a max retry time directly, rather than a count of retries? retry counts are complicated because, as you are showing here, they have a cross-flag interaction with the timeout, and also are going to have very different behavior depending on whether the call took a long time (a timeout) vs a short time (a service queue overflow) http://gerrit.cloudera.org:8080/#/c/12299/4/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: http://gerrit.cloudera.org:8080/#/c/12299/4/common/thrift/generate_error_codes.py@403 PS4, Line 403: 3s ms? -- To view, visit http://gerrit.cloudera.org:8080/12299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType:
[Impala-ASF-CR](2.x) Fix zsh issue in set-pythonpath.sh
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12806 ) Change subject: Fix zsh issue in set-pythonpath.sh .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3935/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12806 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: Ia902891ab36f3aee96a53aa105cc5775321d0058 Gerrit-Change-Number: 12806 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 21 Mar 2019 04:41:23 + Gerrit-HasComments: No
[Impala-ASF-CR](2.x) IMPALA-6031: Fix executor node count in distributed plans
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12801 ) Change subject: IMPALA-6031: Fix executor node count in distributed plans .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3934/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12801 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: I44af6b40099a495e13a0a5dc72c491d486d23aa8 Gerrit-Change-Number: 12801 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 21 Mar 2019 04:40:38 + Gerrit-HasComments: No
[Impala-ASF-CR](2.x) IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12800 ) Change subject: IMPALA-6223: Gracefully handle malformed 'with' queries in impala-shell .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3933/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12800 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: Ibb1e9238ac67b8ad3b2caa1748a18b04f384802d Gerrit-Change-Number: 12800 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Pooja Nilangekar Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 21 Mar 2019 04:40:00 + Gerrit-HasComments: No
[Impala-ASF-CR](2.x) IMPALA-6988: Implement ALTER TABLE/VIEW SET OWNER
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12799 ) Change subject: IMPALA-6988: Implement ALTER TABLE/VIEW SET OWNER .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3932/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12799 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: Ia1b75b1590b16eb0c2ba326d07ee3fd9897c27d1 Gerrit-Change-Number: 12799 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Thu, 21 Mar 2019 04:39:21 + Gerrit-HasComments: No
[Impala-ASF-CR](2.x) IMPALA-6642 (Part 2): clean up start-impala-cluster.py
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12798 ) Change subject: IMPALA-6642 (Part 2): clean up start-impala-cluster.py .. Patch Set 1: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3931/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: I60169203c61ae6bc0a3ccd3dea355799b603efe5 Gerrit-Change-Number: 12798 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 21 Mar 2019 04:38:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 15: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 15 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 04:18:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. Patch Set 17: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12632/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/12632/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@72 PS16, Line 72:// Hive service definition does not have a concept of server. So we define : // server to mean all access to all resource sets. > It would be good if Hive could support it, but that might actually be a big k. Let's come back to this at some point... I can also see the argument that those things may be better off living in a separate Impala-specific service, if it's possible to have a single plugin authorize against two service models. Or, maybe those things would be better off actually passed as a config flag to the impala cluster itself at startup time rather than being centrally administrated, because they are more about service administration than about the underlying data. (eg in a multi-cluster environment, if I "own" an impalad cluster, I should be able to shut it down, but that doesn't imply any permission for me to access any data) -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 04:18:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8014: Revise FK/PK cardinality estimation
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12535 ) Change subject: IMPALA-8014: Revise FK/PK cardinality estimation .. Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/12535/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12535/5//COMMIT_MSG@28 PS5, Line 28: including some TPC-H plans did you have any chance to compare the before-and-after plans from a performance perspective? Same with DS, where it seems a few plans changed? (not that we should slavishly adhere to those benchmarks if we think the new heuristic is better, but if we've regressed it would be good to be explicit about how much) http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@327 PS5, Line 327:* By definition, the detail (fk) table is on the left, the master (pk) table I haven't seen the terminology "detail" and "master" being used anywhere else in the planner. Are these standard DB terminology? I think we should avoid introducing new terms not used elsewhere in the codebase. To me, using the terms "fact" and "dimension" might make sense, or just stick to LHS and RHS, since it's conventional throughput Impala that RHS is the smaller "build" table whereas LHS is the larger "probe" table. http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@331 PS5, Line 331:* can assume a cap on the NDV of a joint FK: it must be no larger than the NDV when you say "NDV of a joint FK", should that be "of a composite FK"? ie the case you're differentiating is when the equijoin conjunct is on multiple columns? By some quick googling, the term "composite key" seems to be the most common name for this case http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@332 PS5, Line 332:* of the FK, which is (by definition) the same as the master table cardinality. isn't it by definition _bound_ by the master table cardinality, not the same as? ie there may be items in the PK table that are not referenced, so its cardinality may be larger. http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@339 PS5, Line 339:* 3. Containment: that if the FK side has fewer keys than the PK side, all excuse my ignorance, but is "containment" the common term for this assumption? To me, this is "referential integrity" -- ie that all FKs reference an existing row in the PK table. http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@351 PS5, Line 351: // that the RHS is a fact (master, FK) table. again confused on terminology here (and seems there is at least one typo). Above, it says that the table with the FK is on the left. As such, isn't the LHS the fact table (table with the FK)? And the right side is the dimension table (table with the PK)? http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@353 PS5, Line 353: |RHS key| I think this and some of the text below would be easier to follow if we don't overload |...| to mean both count and NDV. i.e. here what you mean is NDV(RHS key) = |RHS table| This also matches the notation used in some of the block comments above http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@355 PS5, Line 355: In the typical case Isn't this one of the assumptions above? (the "containment"/referential integrity one) http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@355 PS5, Line 355: very nit: every http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@360 PS5, Line 360: |N| what is |N| here? In a PK relationship isn't the probability of a detail (fact) row finding a match in an unfiltered dimension table always 1? http://gerrit.cloudera.org:8080/#/c/12535/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@370 PS5, Line 370: |D" typo at end of line here (that should say |D|, right?) One question: would it not be valid to just say this is a degenerate case of the compound key calculation, where the product of one number is just the number itself? ie: |D.fk'| = min(|D.fk|, |M|) * |D'|/|D| That's slightly different than what you have here, but in a "true" PK relationship, we are guaranteed that |D.fk| <= |M|, and if our assumption was off by a bit, we're at still guaranteed that |D.fk| is within some bound of |M| due to the fact that we're in this code path anyway. That would allow us to simplify the code below a bit, right? http://gerrit.cloudera
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 15: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2495/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 15 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 01:29:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. Patch Set 17: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2497/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 01:35:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 9: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2496/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 01:32:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. Patch Set 17: (2 comments) http://gerrit.cloudera.org:8080/#/c/12632/16/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/12632/16/be/src/service/frontend.cc@47 PS16, Line 47: with Range > this is only required if authorization is enabled, right? Yes. I guess instead of putting (Required) here can be confusing. Done. http://gerrit.cloudera.org:8080/#/c/12632/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/12632/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@72 PS16, Line 72:// Hive service definition does not have a concept of server. So we define : // server to mean all access to all resource sets. > Is this something we need to address? I can't recall all the cases in which It would be good if Hive could support it, but that might actually be a big change from the existing Ranger plugin for Hive. Technically having server as a root is better since we only need to make a single call instead of having 3 calls. We have certain statements that require server-level authorization, such as shutdown command, invalidate metadata (the global), and there are more I think. -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 01:05:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Fredy Wijaya has uploaded a new patch set (#17). ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. IMPALA-8100: Add initial support for Ranger This patch adds an initial support for Ranger that can be enabled via the following flags in both impalad and catalogd to do enforcement. - ranger_service_type=hive - ranger_app_id=some_app_id - authorization_factory_class=\ org.apache.impala.authorization.ranger.RangerAuthorizationFactory The Ranger plugin for Impala uses Hive service definition to allow sharing Ranger policies between Hive and Impala. Temporarily the REFRESH privilege uses "read" access type and it will be updated in the later patch once Ranger supports "refresh" access type. There's a change in DESCRIBE privilege requirement to use ANY privilege instead of VIEW_METADATA privilege as the first-level check to play nicely with Ranger. This is not a security risk since the column-level filtering logic after the first-level check will use VIEW_METADATA privilege to filter out unauthorized column access. In other words, DESCRIBE may return an empty result instead of an authorization error as long as there exists any privilege in the given table. This patch updates AuthorizationStmtTest with a parameterized test that runs the tests against Sentry and Ranger. Testing: - Updated AuthorizationStmtTest with Ranger - Ran all FE tests - Ran all E2E authorization tests Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 --- M be/src/service/frontend.cc M be/src/util/backend-gflag-util.cc M bin/rat_exclude_files.txt M common/thrift/BackendGflags.thrift M fe/pom.xml M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationProvider.java M fe/src/main/java/org/apache/impala/authorization/DefaultAuthorizableFactory.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/Privilege.java A fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java A fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationConfig.java A fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java A fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaPlugin.java A fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaResourceBuilder.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java M fe/src/main/java/org/apache/impala/service/BackendConfig.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java M fe/src/test/java/org/apache/impala/common/FrontendFixture.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java A fe/src/test/resources/ranger-hive-audit.xml A fe/src/test/resources/ranger-hive-security.xml M impala-parent/pom.xml M testdata/bin/create-load-data.sh M testdata/cluster/.gitignore A testdata/cluster/ranger/setup/impala_service.json A testdata/cluster/ranger/setup/impala_user.json.template 38 files changed, 1,131 insertions(+), 237 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/32/12632/17 -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 17 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2494/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 6 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 21 Mar 2019 00:52:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 10: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3930/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 00:49:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12781 )
Change subject: IMPALA-8317: Add support for list type flags in Impala shell
config file
..
Patch Set 9: Code-Review+2
(3 comments)
Carry Bharath's +2.
http://gerrit.cloudera.org:8080/#/c/12781/8/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/12781/8/shell/option_parser.py@78
PS8, Line 78: result[option] = value.split('\n')
> Kinda curious why you have to split on '\n' here again. Looks like you are
Yeah this probably has something to do with the implementation of ConfigParser.
See
https://stackoverflow.com/questions/15848674/how-to-configparse-a-file-keeping-multiple-values-for-identical-keys#comment91291420_15848928.
http://gerrit.cloudera.org:8080/#/c/12781/8/shell/option_parser.py@86
PS8, Line 86: class MultiOrderedDict(OrderedDict):
> Add a doc? (why this is used)
Done
http://gerrit.cloudera.org:8080/#/c/12781/8/tests/shell/good_impalarc
File tests/shell/good_impalarc:
http://gerrit.cloudera.org:8080/#/c/12781/8/tests/shell/good_impalarc@4
PS8, Line 4: keyval=msg1=hello
> nit: Move the comment to the opt_parser where we parse this?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12781
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73
Gerrit-Change-Number: 12781
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Anonymous Coward (395)
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 21 Mar 2019 00:48:36 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. IMPALA-8317: Add support for list type flags in Impala shell config file This patch adds support for list type flags in Impala shell config file, i.e. those that use action="append", such as --var and --query_option. To make it less error-prone, this patch also updates the logic for bool flags in the config file to also look at the correct type from the argument parser instead of relying on whether or not the default values are set in impala_shell_config_defaults.py. Testing: - Added a new test for list type flags - Ran all shell E2E tests Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 --- M shell/impala_shell.py M shell/option_parser.py M tests/shell/good_impalarc M tests/shell/test_shell_commandline.py 4 files changed, 48 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12781/9 -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 00:49:14 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 15: Code-Review+2 (1 comment) Carry Todd's +2. Will merge once https://gerrit.cloudera.org/c/12632/ is merged. http://gerrit.cloudera.org:8080/#/c/12684/14/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12684/14/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@130 PS14, Line 130: String roleName = Iterables.getOnlyElement(params.getRole_names()); > you didn't like my suggestion of using Iterables.getOnlyElement? That would Initially I thought it was something in the JDK. I just realized you meant the one from Guava. TIL :) Done. -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 15 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 21 Mar 2019 00:43:45 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5843: Use page index in Parquet files to skip pages
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12065 ) Change subject: IMPALA-5843: Use page index in Parquet files to skip pages .. Patch Set 7: (19 comments) I spent a bit of time in the guts of the parquet code. I ran some benchmarks against the TPC-H data and didn't find any regressions. On the TPC-H data it's surprisingly hard to find queries that benefit from the column indices, because the data in most columns of lineitem is low-NDV and/or uniformly distributed, except for l_orderkey, which is totally ordered so the existing min/max stats are pretty effective. Actually, now that I think about it, I might have loaded data with an Impala version that doesn't write page indexes by default, so maybe that explains it (but I can't tell directly from the profile) http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.h File be/src/exec/parquet/hdfs-parquet-scanner.h: http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.h@424 PS7, Line 424: std::vector> candidate_ranges_; This is actually confusing because at line 638 of the .cc, this being empty signals that all the rows were eliminated. I think (candidate_ranges_.empty() && page_filtering_) means that all pages were filtered out, but (candidate_ranges_.empty() && !page_filtering_) means that no pages were filtered out. I guess the comment is accurate once we're actually scanning the data, but it's misleading about the intermediate values of these variables during and after ProcessPageIndex() returns. I have a couple of ideas for how to clarify it. I'd be ok with either: 1. Mention here that if all rows were eliminated, then the row group is skipped immediately after evaluating the page index. 2. Make the data flow clearer for ProcessPageIndex() clearer by having candidate_ranges_ and page_filtering_ be explicit output arguments. Then only the caller of ProcessPageIndex() actually sets candidate_ranges_. http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.h@427 PS7, Line 427: bool page_filtering_ = false; This variable is only used in the NextRowGroup()->ProcessPageIndex()->EvaluatePageIndex() call change, so could just be passed as an argument. It would be good to reduce the amount of state and make the data flow explicit. http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc File be/src/exec/parquet/hdfs-parquet-scanner.cc: http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@119 PS7, Line 119: num_stats_filtered_row_groups_by_page_index_counter_ = I think we should probably track the number of row groups with page indices. Otherwise we don't know whether filtering was ineffective because the indices were missing or because they weren't selective http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@708 PS7, Line 708: //TODO: Try to remove code duplications between this and I had a look at this and the control flow is different enough that it would be hard to combine them without it being hard to follow. There might be some opportunity to condense the code or factor out logical functions to make the core algorithm easier to understand. I have some suggestions along those lines below. Some of this is a matter of taste (and you have good taste IMO) so please take or leave as you see fit. http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@716 PS7, Line 716: bool pos_field; Nit: Can combine the declarations of pos_field and missing_field onto one line. http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@719 PS7, Line 719: schema_resolver_->ResolvePath(slot_desc->col_path(), &node, &pos_field, Any reason to handle pos_field=true differently from EvaluateStatsConjuncts()? http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@721 PS7, Line 721: Unnecessary blank line. http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@724 PS7, Line 724: int col_idx = node->col_idx; We could improve compactness of code by just inlining col_idx http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@734 PS7, Line 734: const parquet::ColumnOrder* col_order = nullptr; 733-735 could be a single ternary statement http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@736 PS7, Line 736: ColumnStatsReader stats_reader(col_chunk, col_type, col_order, *node->element); 736-739 could maybe be a helper function http://gerrit.cloudera.org:8080/#/c/12065/7/be/src/exec/parquet/hdfs-parquet-scanner.cc@747 PS7, Line 747: bool value_read = false; 747-762 could probably be a static helper function, e
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Fredy Wijaya has uploaded a new patch set (#15). ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. IMPALA-7917 (Part 3): Decouple Sentry from Impala The third part of this patch introduces an interface called AuthorizationManager to perform authorization management related functions, such as granting/revoking a privilege, etc. Some existing authorization management methods have been refactored to reduce the need for if/else conditions to perform certain actions. This patch moves code related to Sentry authorization management code to: - SentryCatalogdAuthorizationManager - SentryImpaladAuthorizationManager The split makes it easier to differentiate between authorization DDL operations performed in Catalogd vs Impalad. This patch does not implement AuthorizationManager for Ranger. This patch has no functionality change. Testing: - Ran all FE tests - Ran all E2E authorization tests Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 --- M fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java A fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java A fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java A fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/main/java/org/apache/impala/util/ClassUtil.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java 23 files changed, 1,125 insertions(+), 430 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/12684/15 -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 15 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] Add missing sudo call to `service postgresql start`
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12724 ) Change subject: Add missing sudo call to `service postgresql start` .. Patch Set 2: Code-Review+2 Carry +2 -- To view, visit http://gerrit.cloudera.org:8080/12724 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3859ad1308b7cd1d483d043b9f71d9f95abdf75 Gerrit-Change-Number: 12724 Gerrit-PatchSet: 2 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Laszlo Gaal Gerrit-Comment-Date: Thu, 21 Mar 2019 00:27:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12781 )
Change subject: IMPALA-8317: Add support for list type flags in Impala shell
config file
..
Patch Set 8: Code-Review+2
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py@77
PS4, Line 77: "append":
> Yeah because we parse the INI file using the parser. So the "options" varia
Ah gotcha
http://gerrit.cloudera.org:8080/#/c/12781/8/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/12781/8/shell/option_parser.py@78
PS8, Line 78: result[option] = value.split('\n')
Kinda curious why you have to split on '\n' here again. Looks like you are
already extend() ing in the __set_item__ override below, so I'd guess value
here should be a list? Haven't dug into it but curious why.
http://gerrit.cloudera.org:8080/#/c/12781/8/shell/option_parser.py@86
PS8, Line 86: class MultiOrderedDict(OrderedDict):
Add a doc? (why this is used)
http://gerrit.cloudera.org:8080/#/c/12781/8/tests/shell/good_impalarc
File tests/shell/good_impalarc:
http://gerrit.cloudera.org:8080/#/c/12781/8/tests/shell/good_impalarc@4
PS8, Line 4: # TODO: IMPALA-8330: Use flag names instead of dest names.
nit: Move the comment to the opt_parser where we parse this?
http://gerrit.cloudera.org:8080/#/c/12781/8/tests/shell/good_impalarc@9
PS8, Line 9:
May be also add another keyval in the end (to make sure it gets parsed too)?
--
To view, visit http://gerrit.cloudera.org:8080/12781
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73
Gerrit-Change-Number: 12781
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Anonymous Coward (395)
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Thu, 21 Mar 2019 00:22:40 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Vihang Karajgaonkar has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. IMPALA-8312 : Alter database operations have race condition This patch fixes a race condition in the alter database implementation in the catalogOpExecutor. The original implementation did a in-place modification of the metastore database object in the Db. This can lead to partially updated database object becoming visible to a reading thread causing potential problems. In order to fix the race, the patch makes a copy of the existing database object, modifies the copy and then atomically switches the actual database object with the modified copy. This is done while holding the metastoreddlLock, and then taking the write lock on the catalog version object which makes it consistent with the other catalog write operations. Added a test which consistently reproduces the race. The test creating many reader threads and a writer thread which continuously keeps changing the owner name and its type by issuing a alter database operation. The test fails without the patch. After the patch the test passes. The race also applies to the alter database set comment operation, although its hard to write a test for that code-path. Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc --- M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java M fe/src/main/java/org/apache/impala/catalog/Db.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java A fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java 4 files changed, 300 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/12789/6 -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 6 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] IMPALA-8100: Add initial support for Ranger
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12632 ) Change subject: IMPALA-8100: Add initial support for Ranger .. Patch Set 16: (2 comments) http://gerrit.cloudera.org:8080/#/c/12632/16/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/12632/16/be/src/service/frontend.cc@47 PS16, Line 47: (Required) this is only required if authorization is enabled, right? http://gerrit.cloudera.org:8080/#/c/12632/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java: http://gerrit.cloudera.org:8080/#/c/12632/16/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@72 PS16, Line 72:// Hive service definition does not have a concept of server. So we define : // server to mean all access to all resource sets. Is this something we need to address? I can't recall all the cases in which we need to do server-level authorization -- To view, visit http://gerrit.cloudera.org:8080/12632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8cad9e609d20aae1ff645c84fd58a02afee70276 Gerrit-Change-Number: 12632 Gerrit-PatchSet: 16 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Mar 2019 23:52:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 14: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12684/14/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12684/14/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@130 PS14, Line 130: String groupName = params.getGroup_names().iterator().next(); you didn't like my suggestion of using Iterables.getOnlyElement? That would ensure there is exactly one group passed here and not more than one. -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 14 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Mar 2019 23:36:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6326: part 2: remove fetch thread in stress test
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12681 ) Change subject: IMPALA-6326: part 2: remove fetch thread in stress test .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2493/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9afd74e1408823a9e5c0f2628ec9f8aafdcec69 Gerrit-Change-Number: 12681 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Mar 2019 23:00:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. IMPALA-8326: Fix CreateDropRoleStmt::toSql Previously the CreateDropRoleStmt::toSql method would incorrectly order the stringified SQL statement as ROLE CREATE/DROP. This patch will properly order the statementas CREATE/DROP ROLE . Testing: - Ran all FE tests - Added a new test to ToSqlTest for CreateDropRoleStmt Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Reviewed-on: http://gerrit.cloudera.org:8080/12813 Reviewed-by: Fredy Wijaya Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 2 files changed, 15 insertions(+), 1 deletion(-) Approvals: Fredy Wijaya: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 6 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 22:55:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6326: part 2: remove fetch thread in stress test
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12681 ) Change subject: IMPALA-6326: part 2: remove fetch thread in stress test .. Patch Set 6: Rebased and resolved a conflict with the merged version of part 1 -- To view, visit http://gerrit.cloudera.org:8080/12681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If9afd74e1408823a9e5c0f2628ec9f8aafdcec69 Gerrit-Change-Number: 12681 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Mar 2019 22:33:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2492/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 22:41:03 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6326: part 2: remove fetch thread in stress test
Hello Thomas Marshall, David Knupp, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12681 to look at the new patch set (#6). Change subject: IMPALA-6326: part 2: remove fetch thread in stress test .. IMPALA-6326: part 2: remove fetch thread in stress test Ensure cursor is only accessed from a single thread. The means reworking the code so that we check the time limit between fetch calls. Use EXEC_TIME_LIMIT_S as an alternative to the previous multi-threaded cancellation logic - it allows queries to be cancelled even when the client is blocked or slow. This is implemented with the concept of a CancelMechanism that determines *how* a query should be cancelled. Query timeouts (where we want to cancel queries that run longer than expected) are implemented using both cancel mechanisms, in case the client is stuck in fetch or similar. Expected cancellations are implemented with a random mechanism so that both code paths get covered. Testing: Ran a cluster stress test. Ran a couple of single-node stress tests with TPC-H and random queries. Change-Id: If9afd74e1408823a9e5c0f2628ec9f8aafdcec69 --- M tests/stress/concurrent_select.py M tests/stress/query_runner.py 2 files changed, 129 insertions(+), 90 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12681/6 -- To view, visit http://gerrit.cloudera.org:8080/12681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If9afd74e1408823a9e5c0f2628ec9f8aafdcec69 Gerrit-Change-Number: 12681 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: David Knupp Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8158: Retrieve thrift profiles through Impyla
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12530 ) Change subject: IMPALA-8158: Retrieve thrift profiles through Impyla .. Patch Set 5: I'll wait until you've bumped the Impyla version do do a final review. -- To view, visit http://gerrit.cloudera.org:8080/12530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I769a99f0843297dd2b20f2f5b1a9046c97bb131e Gerrit-Change-Number: 12530 Gerrit-PatchSet: 5 Gerrit-Owner: Lars Volker Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 20 Mar 2019 22:30:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2491/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 22:23:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. IMPALA-8325: Leading Unicode comments cause Impala Shell failure. This change fixes a regression introduced by "IMPALA-2195 Improper handling of comments in queries." The Impala Shell parses input text into several strings using the sqlparse library. One of the returned strings is the sql command, this is used to determine the correct do_ method to call. Another of the returned strings is the leading comment, which is a comment that appears before legal sql text. Python2 has strings with multiple encodings. The strings returned from the sqlparse library have the Unicode encoding. Impala Shell converts the sql command string to utf-8 encoding before using it. If the Impala Shell needs to send the sql command to an Impala Coordinator then it (re)constructs the query out of the strings returned by the sqlparse library. This query is sent to the Coordinator via Beeswax protocol. The query is converted to an ascii string before being sent. The conversion can fail if the leading comment string contains Unicode characters, which can't be directly converted to ascii. So the trigger for the bug is that the leading comment contains Unicode. The fix is that the leading comment string should be converted to utf-8 in the same way as the sql command. TESTING: Ran all end -to-end tests. Added two test cases to tests/shell/test_shell_interactive.py Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Reviewed-on: http://gerrit.cloudera.org:8080/12812 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M shell/impala_shell.py M tests/shell/test_shell_interactive.py 2 files changed, 8 insertions(+), 1 deletion(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. Patch Set 3: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 22:18:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. IMPALA-8317: Add support for list type flags in Impala shell config file This patch adds support for list type flags in Impala shell config file, i.e. those that use action="append", such as --var and --query_option. To make it less error-prone, this patch also updates the logic for bool flags in the config file to also look at the correct type from the argument parser instead of relying on whether or not the default values are set in impala_shell_config_defaults.py. Testing: - Added a new test for list type flags - Ran all shell E2E tests Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 --- M shell/option_parser.py M tests/shell/good_impalarc M tests/shell/test_shell_commandline.py 3 files changed, 45 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12781/8 -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py@77 PS4, Line 77: "append": > Do we need this fancy MultiOrderedDict here? Can't we just do Yeah because we parse the INI file using the parser. So the "options" variable is what comes from the parser and without customizing the parser, it will not handle duplicate keys. http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc File tests/shell/good_impalarc: http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc@4 PS4, Line 4: # TODO > Thanks, mind adding a TODO there with the jira reference? (so that others d Done -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 22:15:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12781 )
Change subject: IMPALA-8317: Add support for list type flags in Impala shell
config file
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py@77
PS4, Line 77: value.split(",")
> Fair point. The reason I did was because we used ConfigParser which doesn't
Do we need this fancy MultiOrderedDict here? Can't we just do
elif opt.action == append:
if not option in result.keys():
result[option] = []
result[option].append(value)
Does that not work for some reason?
http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc
File tests/shell/good_impalarc:
http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc@4
PS4, Line 4: keyval
> Yeah, Vincent and I were talking about the same concern here. I agree it's
Thanks, mind adding a TODO there with the jira reference? (so that others don't
get confused when they see it for the first time, like me :-))
--
To view, visit http://gerrit.cloudera.org:8080/12781
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73
Gerrit-Change-Number: 12781
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Anonymous Coward (395)
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 22:08:10 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8328: Add missing ToSql test cases for authorization statements
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12817 )
Change subject: IMPALA-8328: Add missing ToSql test cases for authorization
statements
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
File fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java:
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java@44
PS1, Line 44: if (isGrantStmt_) {
: return String.format("GRANT ROLE %s TO GROUP %s",
roleName_, groupName_);
: } else {
: return String.format("REVOKE ROLE %s FROM GROUP %s",
roleName_, groupName_);
: }
nit: can be simplified by using ternary operation.
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
File fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java:
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java@297
PS1, Line 297: public TPrivilegeScope getScope_() {
: return scope_;
: }
:
: public TableName getTableName_() {
: return tableName_;
: }
:
: public HdfsUri getUri_() {
: return uri_;
: }
:
: public String getDbName_() {
: return dbName_;
: }
:
: public String getServerName_() {
: return serverName_;
: }
nit: method names should not have _ at the end. Impala code style is to put the
methods into a single line if possible.
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
File fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java:
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@93
PS1, Line 93: switch (privilegeSpec_.getScope_()) {
: case SERVER:
: break;
: case DATABASE:
: sb.append(String.format(" %s",
privilegeSpec_.getDbName_()));
: break;
: case TABLE:
: sb.append(String.format(" %s",
privilegeSpec_.getTableName_()));
: break;
: case URI:
: sb.append(String.format(" '%s'",
privilegeSpec_.getUri_()));
: break;
: }
add default or else the compiler may give a warning.
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1478
PS1, Line 1478: try {
: catalog_.addRole(testRole);
: testToSql(ctx, String.format("GRANT SELECT ON SERVER
server1 TO %s", testRole));
: testToSql(ctx, String.format("GRANT SELECT ON SERVER TO
%s", testRole),
: String.format("GRANT SELECT ON SERVER server1 TO %s",
testRole));
: testToSql(ctx, String.format(
: "GRANT ALL ON SERVER server1 TO %s WITH GRANT
OPTION", testRole));
:
: testToSql(ctx, String.format("REVOKE SELECT ON SERVER
server1 FROM %s", testRole));
: testToSql(ctx, String.format("REVOKE SELECT ON SERVER
FROM %s", testRole),
: String.format("REVOKE SELECT ON SERVER server1 FROM
%s", testRole));
: testToSql(ctx, String.format(
: "REVOKE GRANT OPTION FOR ALL ON SERVER server1 FROM
%s", testRole));
: } finally {
: catalog_.removeRole(testRole);
: }
We can have a better coverage by iterating this code over list of privileges
via Privilege.values(). Can we also test all the supported objects, i.e.
server, database, table, column, uri?
http://gerrit.cloudera.org:8080/#/c/12817/1/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1520
PS1, Line 1520: testToSql(ctx, String.format("SHOW GRANT ROLE %s",
testRole));
: testToSql(ctx, String.format("SHOW GRANT USER %s",
testUser));
: testToSql(ctx, String.format("SHOW GRANT ROLE %s ON
SERVER", testRole));
: testToSql(ctx, String.format("SHOW GRANT ROLE %s ON
DATABASE functional",
: testRole));
: testToSql(ctx, String.format(
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py@77 PS4, Line 77: "append": > Not super sure if this is the right way. Why always a ','? Fair point. The reason I did was because we used ConfigParser which doesn't allow duplicate keys by default. There's a way to customize it, but not as flexible as the one in Python 3. Anyway I updated the CR to fix this. The split is still necessary because in Python 2.6 and 2.7, the duplicate keys will be appended with \n. Done. http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc File tests/shell/good_impalarc: http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc@4 PS4, Line 4: keyval > I'm a little bit surprised by this. I'd assume these should map to the actu Yeah, Vincent and I were talking about the same concern here. I agree it's also super confusing and we should use the flag name and not the dest name. Unfortunately it's been like that since the beginning. We definitely should fix this though. I filed a JIRA here: https://issues.apache.org/jira/browse/IMPALA-8330 -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 21:39:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Fredy Wijaya has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/12781 ) Change subject: IMPALA-8317: Add support for list type flags in Impala shell config file .. IMPALA-8317: Add support for list type flags in Impala shell config file This patch adds support for list type flags in Impala shell config file, i.e. those that use action="append", such as --var and --query_option. To make it less error-prone, this patch also updates the logic for bool flags in the config file to also look at the correct type from the argument parser instead of relying on whether or not the default values are set in impala_shell_config_defaults.py. Testing: - Added a new test for list type flags - Ran all shell E2E tests Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 --- M shell/option_parser.py M tests/shell/good_impalarc M tests/shell/test_shell_commandline.py 3 files changed, 44 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/12781/7 -- To view, visit http://gerrit.cloudera.org:8080/12781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73 Gerrit-Change-Number: 12781 Gerrit-PatchSet: 7 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Anonymous Coward (395) Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12789 )
Change subject: IMPALA-8312 : Alter database operations have race condition
..
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12789/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12789/5//COMMIT_MSG@12
PS5, Line 12: partially updated database object becoming visible to a reading
thread causing
nit: line overflows
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java
File fe/src/main/java/org/apache/impala/catalog/Db.java:
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/main/java/org/apache/impala/catalog/Db.java@71
PS5, Line 71: AtomicReference
Is there any advantage in using AtomicReference when any changes to the parent
Db only occur under an exclusive version lock?
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java
File fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java:
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@80
PS5, Line 80: public static void setUpTest() throws ImpalaException {
Add method doc?
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@131
PS5, Line 131: ReaderTask
ValidateDbOwnerTask or something?
http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@149
PS5, Line 149: WriterTask
Better name?
--
To view, visit http://gerrit.cloudera.org:8080/12789
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc
Gerrit-Change-Number: 12789
Gerrit-PatchSet: 5
Gerrit-Owner: Vihang Karajgaonkar
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 21:18:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8328: Add missing ToSql test cases for authorization statements
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12817 ) Change subject: IMPALA-8328: Add missing ToSql test cases for authorization statements .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2490/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I828d0459b6c92f7f15006e2353ce108093aa9dab Gerrit-Change-Number: 12817 Gerrit-PatchSet: 1 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 21:09:15 + Gerrit-HasComments: No
[Impala-ASF-CR] Use `wget http://169.254.169.254/` to determine if we're running in aws
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12727 ) Change subject: Use `wget http://169.254.169.254/` to determine if we're running in aws .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2489/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iddb2574dbcb3f97cf697095d1777e51ce463b205 Gerrit-Change-Number: 12727 Gerrit-PatchSet: 3 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Hector Acosta Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Laszlo Gaal Gerrit-Comment-Date: Wed, 20 Mar 2019 21:04:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8317: Add support for list type flags in Impala shell config file
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/12781 )
Change subject: IMPALA-8317: Add support for list type flags in Impala shell
config file
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py
File shell/option_parser.py:
http://gerrit.cloudera.org:8080/#/c/12781/4/shell/option_parser.py@77
PS4, Line 77: value.split(",")
Not super sure if this is the right way. Why always a ','?
I'd assume the way it works with append stuff is that you need to provide each
option separately.
For ex: --var=foo=bar --var=x=y
Shouldn't we keep that behavior consistent with the rc files here?
For example, the above should translate to
[impala]
...
keyval=foo=bar
keyval=x=y
Thoughts?
http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc
File tests/shell/good_impalarc:
http://gerrit.cloudera.org:8080/#/c/12781/4/tests/shell/good_impalarc@4
PS4, Line 4: keyval
I'm a little bit surprised by this. I'd assume these should map to the actual
shell option rather than the "dest" string. How'd the customer know the "dest"
variables here without having to look at the shell code? Thoughts?
--
To view, visit http://gerrit.cloudera.org:8080/12781
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I824ca15b4e1064a391b13deef9cecd34c928ef73
Gerrit-Change-Number: 12781
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Anonymous Coward (395)
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 20:40:56 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8328: Add missing ToSql test cases for authorization statements
Austin Nobis has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12817 Change subject: IMPALA-8328: Add missing ToSql test cases for authorization statements .. IMPALA-8328: Add missing ToSql test cases for authorization statements ToSql test cases were added for the following authorization statement classes: ShowRolesStmt, ShowGrantPrincipalStmt, GrantRevokeRoleStmt, GrantRevokePrivStmt. In addition, the ShowGrantPrincipalStmt::toSql and GrantRevokeRoleStmt::toSql statements were found to be incorrect during testing and were updated to align with the grammar. Testing: - Ran all FE tests - Added new tests to ToSqlTest for the listed statements Change-Id: I828d0459b6c92f7f15006e2353ce108093aa9dab --- M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 4 files changed, 109 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/17/12817/1 -- To view, visit http://gerrit.cloudera.org:8080/12817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I828d0459b6c92f7f15006e2353ce108093aa9dab Gerrit-Change-Number: 12817 Gerrit-PatchSet: 1 Gerrit-Owner: Austin Nobis
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 ) Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 20:27:57 + Gerrit-HasComments: No
[Impala-ASF-CR] Use `wget http://169.254.169.254/` to determine if we're running in aws
Hello Laszlo Gaal, Jim Apple, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12727 to look at the new patch set (#3). Change subject: Use `wget http://169.254.169.254/` to determine if we're running in aws .. Use `wget http://169.254.169.254/` to determine if we're running in aws https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/identify_ec2_instances.html lists the above endpoint as the first choice. In my running instance `dmidecode -s bios-version` prints out: 1.0 dmidecode --string system-uuid|grep ^ec2 seems like a valid alternative here. I removed the `grep` assertions since it's possible that images may have already configured ntp here. Change-Id: Iddb2574dbcb3f97cf697095d1777e51ce463b205 --- M bin/bootstrap_system.sh 1 file changed, 2 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/27/12727/3 -- To view, visit http://gerrit.cloudera.org:8080/12727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iddb2574dbcb3f97cf697095d1777e51ce463b205 Gerrit-Change-Number: 12727 Gerrit-PatchSet: 3 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Hector Acosta Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Laszlo Gaal
[Impala-ASF-CR] IMPALA-8312 : Alter database operations have race condition
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12789 ) Change subject: IMPALA-8312 : Alter database operations have race condition .. Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java File fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java: http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@66 PS5, Line 66: private static int numReaders_ = 10; : // number of writer threads which change the database. We only need one currently : // since all the alterDatabase calls are serialized by metastoreDdlLock_ in : // CatalogOpExecutor : private static int numWriters_ = 1; Can be static final. Use upper case for static final variable names. http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@72 PS5, Line 72: // barrier to make sure that readers and writers start at the same time : private static final CyclicBarrier barrier_ = : new CyclicBarrier(numReaders_ + numWriters_); : // toggle switch to change the database owner from user_1 to user_2 in each : // consecutive alter database call : private static final AtomicBoolean toggler_ = new AtomicBoolean(false); I feel like all these variables should not be static especially since we may add another test in the future and need to reset the toggler_, etc. http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@132 PS5, Line 132: nit: remove new line http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@150 PS5, Line 150: nit: remove new line http://gerrit.cloudera.org:8080/#/c/12789/5/fe/src/test/java/org/apache/impala/catalog/AlterDatabaseTest.java@224 PS5, Line 224: throw new Exception should we just use fail() instead of throwing an exception? -- To view, visit http://gerrit.cloudera.org:8080/12789 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32c8c96a6029bf9d9db37ea8315f6c9603b5a2fc Gerrit-Change-Number: 12789 Gerrit-PatchSet: 5 Gerrit-Owner: Vihang Karajgaonkar Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:46:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2487/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 4 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:51:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2488/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:49:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3929/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:47:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 5: Code-Review+2 Thanks for fixing this and added a missing test. LGTM. -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:47:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Austin Nobis has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. IMPALA-8326: Fix CreateDropRoleStmt::toSql Previously the CreateDropRoleStmt::toSql method would incorrectly order the stringified SQL statement as ROLE CREATE/DROP. This patch will properly order the statementas CREATE/DROP ROLE . Testing: - Ran all FE tests - Added a new test to ToSqlTest for CreateDropRoleStmt Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a --- M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 2 files changed, 15 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12813/5 -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12813/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12813/4//COMMIT_MSG@10 PS4, Line 10: ROL > nit: add space before? Done -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:23:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2486/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:22:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12813/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12813/4//COMMIT_MSG@10 PS4, Line 10: ROLE nit: add space before? -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 4 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:21:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2485/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:20:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Austin Nobis has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. IMPALA-8326: Fix CreateDropRoleStmt::toSql Previously the CreateDropRoleStmt::toSql method would incorrectly order the stringified SQL statement as ROLE CREATE/DROP. This patch will properly order the statementas CREATE/DROP ROLE . Testing: - Ran all FE tests - Added a new test to ToSqlTest for CreateDropRoleStmt Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a --- M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 2 files changed, 15 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12813/4 -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 4 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2484/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:07:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3928/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:07:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:07:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:06:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Andrew Sherman has posted comments on this change. (
http://gerrit.cloudera.org:8080/12812 )
Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell
failure.
..
Patch Set 1:
(4 comments)
Thanks Fredy
http://gerrit.cloudera.org:8080/#/c/12812/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12812/1//COMMIT_MSG@23
PS1, Line 23:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/12812/1//COMMIT_MSG@30
PS1, Line 30:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/12812/1/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/12812/1/shell/impala_shell.py@1339
PS1, Line 1339: (leading_comment)
> nit: unnecessary parentheses in python
Thanks
http://gerrit.cloudera.org:8080/#/c/12812/1/shell/impala_shell.py@1340
PS1, Line 1340: leading_comment.encode('utf-8')
> I missed this in my previous patch :( Thanks for the fix!
Thanks for the quick review!
--
To view, visit http://gerrit.cloudera.org:8080/12812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944
Gerrit-Change-Number: 12812
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 18:04:43 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Andrew Sherman has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/12812 ) Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. IMPALA-8325: Leading Unicode comments cause Impala Shell failure. This change fixes a regression introduced by "IMPALA-2195 Improper handling of comments in queries." The Impala Shell parses input text into several strings using the sqlparse library. One of the returned strings is the sql command, this is used to determine the correct do_ method to call. Another of the returned strings is the leading comment, which is a comment that appears before legal sql text. Python2 has strings with multiple encodings. The strings returned from the sqlparse library have the Unicode encoding. Impala Shell converts the sql command string to utf-8 encoding before using it. If the Impala Shell needs to send the sql command to an Impala Coordinator then it (re)constructs the query out of the strings returned by the sqlparse library. This query is sent to the Coordinator via Beeswax protocol. The query is converted to an ascii string before being sent. The conversion can fail if the leading comment string contains Unicode characters, which can't be directly converted to ascii. So the trigger for the bug is that the leading comment contains Unicode. The fix is that the leading comment string should be converted to utf-8 in the same way as the sql command. TESTING: Ran all end -to-end tests. Added two test cases to tests/shell/test_shell_interactive.py Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 --- M shell/impala_shell.py M tests/shell/test_shell_interactive.py 2 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/12812/2 -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2483/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 2 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 18:04:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12813/3/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/12813/3/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1477 PS3, Line 1477: line has trailing whitespace -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 17:50:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Austin Nobis has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. IMPALA-8326: Fix CreateDropRoleStmt::toSql Previously the CreateDropRoleStmt::toSql method would incorrectly order the stringified SQL statement as ROLE CREATE/DROP. This patch will properly order the statementas CREATE/DROP ROLE . Testing: - Ran all FE tests - Added a new test to ToSqlTest for CreateDropRoleStmt Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a --- M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 2 files changed, 15 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12813/3 -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt::toSql
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12813 ) Change subject: IMPALA-8326: Fix CreateDropRoleStmt::toSql .. Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG@7 PS2, Line 7: CreateDropRoleStmt::toSq > nit: CreateDropRoleStmt::toSql Done http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG@9 PS2, Line 9: CreateDropRoleStmt::toSql metho > nit: CreateDropRoleStmt::toSql Done http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG@15 PS2, Line 15: - Ran all FE tests > mention that you added a test for this. Done http://gerrit.cloudera.org:8080/#/c/12813/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/12813/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1477 PS2, Line 1477: > Put this in try/finally and delete it again afterwards. We don't want this Done -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 17:49:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5051: Add INT64 timestamp write support in Parquet
Lars Volker has posted comments on this change. (
http://gerrit.cloudera.org:8080/12247 )
Change subject: IMPALA-5051: Add INT64 timestamp write support in Parquet
..
Patch Set 9:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/12247/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12247/9//COMMIT_MSG@17
PS9, Line 17: INT96_NANOS
I think the NANOS suffix is somewhat misleading as it suggests that this is
just ns precision with a larger range. Since we also want to discourage use of
this I suggest to name it INT96_DEPRECATED (or INT96_IMPALA since this is where
it came from).
http://gerrit.cloudera.org:8080/#/c/12247/9//COMMIT_MSG@42
PS9, Line 42: net
typo
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/exec/parquet/parquet-metadata-utils.h
File be/src/exec/parquet/parquet-metadata-utils.h:
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/exec/parquet/parquet-metadata-utils.h@61
PS9, Line 61: that the type
nit: that the input type..
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/exec/parquet/parquet-metadata-utils.cc
File be/src/exec/parquet/parquet-metadata-utils.cc:
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/exec/parquet/parquet-metadata-utils.cc@150
PS9, Line 150: parquet::TimestampType timestamp_type;
nit: declare these before using them?
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-test.cc
File be/src/runtime/timestamp-test.cc:
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-test.cc@713
PS9, Line 713: int64_t tm_min_micros, tm_min_millis;
I'd declare that when you use it
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-test.cc@721
PS9, Line 721: // Add 250ns and check the value is rounded down
Maybe prefix each of these comments with a comment or assert of the current
value of min_time. Otherwise one has to count how many times we added 250ns all
the time.
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-test.cc@737
PS9, Line 737: EXPECT_TRUE(min_date.FloorUtcToUnixTimeMicros(&tm_min_micros));
This test does not correspond to the comment above, does it? I.e., it is
rounding towards -?
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-test.cc@740
PS9, Line 740: 250us
The switch between us and ns confused me for a bit, not sure if there's a good
way to make it stand out more.
Also, some of these blocks add to the current value of min_date, while other
blocks reset it. Maybe you can make more clear by not writing to it and using
new variables instead (here and in the other blocks)
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.h@239
PS9, Line 239: 2262
The famous Impala Year-2262 problem being conceived and I get to be part of it.
:)
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.h@375
PS9, Line 375: 1970.01.01
nit: 1970-01-01
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.inline.h
File be/src/runtime/timestamp-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.inline.h@98
PS9, Line 98: 24 * 60 * 60
While you're here, this could probably be a constant, too, given it's used in
other places.
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.inline.h@133
PS9, Line 133:
Nit: I think some of these newlines could go.
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.inline.h@156
PS9, Line 156: static_cast(unixtime_seconds) * NANOS_PER_SEC
nit: indent 4 spaces
http://gerrit.cloudera.org:8080/#/c/12247/9/be/src/runtime/timestamp-value.inline.h@160
PS9, Line 160: || nanos128 > std::numeric_limits::max()) return
false;
nit: If it doesn't fit on one line, it should have braces {}
http://gerrit.cloudera.org:8080/#/c/12247/9/tests/query_test/test_insert_parquet.py
File tests/query_test/test_insert_parquet.py:
http://gerrit.cloudera.org:8080/#/c/12247/9/tests/query_test/test_insert_parquet.py@848
PS9, Line 848: ColumnStats('ts', -1001001, 1001001, 0)
Can you add a test for the case that values don't fit into 64bit nanos?
http://gerrit.cloudera.org:8080/#/c/12247/9/tests/util/get_parquet_metadata.py
File tests/util/get_parquet_metadata.py:
http://gerrit.cloudera.org:8080/#/c/12247/9/tests/util/get_parquet_metadata.py@180
PS9, Line 180: normpath
Why is "normpath" needed here?
http://gerrit.cloudera.org:8080/#/c/12247/9/tests/util/get_parquet_metadata.py@180
PS9, Line 180: table_dir = tmp_dir + "/" +
os.path.basename(os.path.normpath(hdfs_path))
use os.path.join
--
To view, visit http://gerrit.cloudera.org:8080/12247
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageTyp
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12812 )
Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell
failure.
..
Patch Set 1: Code-Review+2
(4 comments)
Just couple nits, but LGTM. Thanks for fixing this!
http://gerrit.cloudera.org:8080/#/c/12812/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12812/1//COMMIT_MSG@23
PS1, Line 23:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/12812/1//COMMIT_MSG@30
PS1, Line 30:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/12812/1/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/12812/1/shell/impala_shell.py@1339
PS1, Line 1339: (leading_comment)
nit: unnecessary parentheses in python
http://gerrit.cloudera.org:8080/#/c/12812/1/shell/impala_shell.py@1340
PS1, Line 1340: leading_comment.encode('utf-8')
I missed this in my previous patch :( Thanks for the fix!
--
To view, visit http://gerrit.cloudera.org:8080/12812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944
Gerrit-Change-Number: 12812
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 17:41:30 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt toSql
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12813 )
Change subject: IMPALA-8326: Fix CreateDropRoleStmt toSql
..
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG@7
PS2, Line 7: CreateDropRoleStmt toSql
nit: CreateDropRoleStmt::toSql
http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG@9
PS2, Line 9: CreateDropRoleStmt class' toSql
nit: CreateDropRoleStmt::toSql
http://gerrit.cloudera.org:8080/#/c/12813/2//COMMIT_MSG@15
PS2, Line 15: - Ran all FE tests
mention that you added a test for this.
http://gerrit.cloudera.org:8080/#/c/12813/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/12813/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1477
PS2, Line 1477: catalog_.addRole("test_role");
Put this in try/finally and delete it again afterwards. We don't want this role
to be available for other tests.
--
To view, visit http://gerrit.cloudera.org:8080/12813
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a
Gerrit-Change-Number: 12813
Gerrit-PatchSet: 2
Gerrit-Owner: Austin Nobis
Gerrit-Reviewer: Austin Nobis
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 17:45:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8325: Leading Unicode comments cause Impala Shell failure.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12812 Change subject: IMPALA-8325: Leading Unicode comments cause Impala Shell failure. .. IMPALA-8325: Leading Unicode comments cause Impala Shell failure. This change fixes a regression introduced by "IMPALA-2195 Improper handling of comments in queries." The Impala Shell parses input text into several strings using the sqlparse library. One of the returned strings is the sql command, this is used to determine the correct do_ method to call. Another of the returned strings is the leading comment, which is a comment that appears before legal sql text. Python2 has strings with multiple encodings. The strings returned from the sqlparse library have the Unicode encoding. Impala Shell converts the sql command string to utf-8 encoding before using it. If the Impala Shell needs to send the sql command to an Impala Coordinator then it (re)constructs the query out of the strings returned by the sqlparse library. This query is sent to the Coordinator via Beeswax protocol. The query is converted to an ascii string before being sent. The conversion can fail if the leading comment string contains Unicode characters, which can't be directly converted to ascii. So the trigger for the bug is that the leading comment contains Unicode. The fix is that the leading comment string should be converted to utf-8 in the same way as the sql command. TESTING: Ran all end -to-end tests. Added two test cases to tests/shell/test_shell_interactive.py Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 --- M shell/impala_shell.py M tests/shell/test_shell_interactive.py 2 files changed, 8 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/12812/1 -- To view, visit http://gerrit.cloudera.org:8080/12812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I8633935b6e0ca33594afd32ad242779555e09944 Gerrit-Change-Number: 12812 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-8326: Fix CreateDropRoleStmt toSql
Austin Nobis has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12813 Change subject: IMPALA-8326: Fix CreateDropRoleStmt toSql .. IMPALA-8326: Fix CreateDropRoleStmt toSql Previously the CreateDropRoleStmt class' toSql method would incorrectly order the stringified SQL statement as ROLE CREATE/DROP. This patch will properly order the statement as CREATE/DROP ROLE . Testing: - Ran all FE tests Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a --- M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java 2 files changed, 9 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/12813/2 -- To view, visit http://gerrit.cloudera.org:8080/12813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I25e4e1f680a6992779a47b5e6ad5092f27ba390a Gerrit-Change-Number: 12813 Gerrit-PatchSet: 2 Gerrit-Owner: Austin Nobis
[native-toolchain-CR] Add documentation for docker-based toolchain builds
Hector Acosta has posted comments on this change. ( http://gerrit.cloudera.org:8080/12810 ) Change subject: Add documentation for docker-based toolchain builds .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/12810/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12810/1//COMMIT_MSG@7 PS1, Line 7: based t > maybe "based" ? Done http://gerrit.cloudera.org:8080/#/c/12810/1//COMMIT_MSG@10 PS1, Line 10: ie > nit: typo Done http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md File docker/README.md: http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md@18 PS1, Line 18: committe > typo: committed Done http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md@26 PS1, Line 26: ubunt > typo: ubuntu Done http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md@35 PS1, Line 35: it is possible to specify urls in the DISTROS : make variable. > Does this mean that specifying this URL would cause the Docker images to be Roughly yes. Except that right now, the Makefile assumes that the images have already been built (if the images do not exist, the build procedure fails.) -- To view, visit http://gerrit.cloudera.org:8080/12810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id971bda58d537fa15ec63004f744d3f730bad170 Gerrit-Change-Number: 12810 Gerrit-PatchSet: 2 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Hector Acosta Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 20 Mar 2019 17:14:30 + Gerrit-HasComments: Yes
[native-toolchain-CR] Add documentation for docker-based toolchain builds
Hello Thomas Marshall, Laszlo Gaal, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12810 to look at the new patch set (#2). Change subject: Add documentation for docker-based toolchain builds .. Add documentation for docker-based toolchain builds This commit adds basic documentation with regard to docker-based toolchain builds. It briefly outlines how to build images and how to use them to build the toolchain. I also added the ability to fetch images from a docker registry. And fixed an 'undefined' variable problem with docker-based builds. Change-Id: Id971bda58d537fa15ec63004f744d3f730bad170 --- M Makefile A docker/README.md M functions.sh 3 files changed, 52 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/10/12810/2 -- To view, visit http://gerrit.cloudera.org:8080/12810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id971bda58d537fa15ec63004f744d3f730bad170 Gerrit-Change-Number: 12810 Gerrit-PatchSet: 2 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 14: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2482/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 14 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Mar 2019 17:04:53 + Gerrit-HasComments: No
[Impala-ASF-CR](2.x) IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12807 ) Change subject: IMPALA-7254: Inconsistent decimal behavior for IN/BETWEEN predicate .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/12807 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: 2.x Gerrit-MessageType: comment Gerrit-Change-Id: I44f6ea45f765be7201630541354c72fda0a8a98d Gerrit-Change-Number: 12807 Gerrit-PatchSet: 2 Gerrit-Owner: Quanlong Huang Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Quanlong Huang Gerrit-Comment-Date: Wed, 20 Mar 2019 17:11:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 ) Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2481/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 16:44:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Fredy Wijaya has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. IMPALA-7917 (Part 3): Decouple Sentry from Impala The third part of this patch introduces an interface called AuthorizationManager to perform authorization management related functions, such as granting/revoking a privilege, etc. Some existing authorization management methods have been refactored to reduce the need for if/else conditions to perform certain actions. This patch moves code related to Sentry authorization management code to: - SentryCatalogdAuthorizationManager - SentryImpaladAuthorizationManager The split makes it easier to differentiate between authorization DDL operations performed in Catalogd vs Impalad. This patch does not implement AuthorizationManager for Ranger. This patch has no functionality change. Testing: - Ran all FE tests - Ran all E2E authorization tests Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 --- M fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java A fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthProvider.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java A fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java A fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java M fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java M fe/src/main/java/org/apache/impala/catalog/Catalog.java M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java M fe/src/main/java/org/apache/impala/service/Frontend.java M fe/src/main/java/org/apache/impala/service/JniCatalog.java M fe/src/main/java/org/apache/impala/service/JniFrontend.java A fe/src/main/java/org/apache/impala/util/ClassUtil.java M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java 23 files changed, 1,125 insertions(+), 430 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/12684/14 -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 14 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon
[native-toolchain-CR] Add documentation for docker-related toolchain builds
Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/12810 ) Change subject: Add documentation for docker-related toolchain builds .. Patch Set 1: (6 comments) Thanks for providing a description for the new build mechanism! I have a few nits (typos and copy editing), with one location that wasn't completely clear to me. http://gerrit.cloudera.org:8080/#/c/12810/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12810/1//COMMIT_MSG@7 PS1, Line 7: related maybe "based" ? http://gerrit.cloudera.org:8080/#/c/12810/1//COMMIT_MSG@10 PS1, Line 10: ei nit: typo http://gerrit.cloudera.org:8080/#/c/12810/1/Makefile File Makefile: http://gerrit.cloudera.org:8080/#/c/12810/1/Makefile@19 PS1, Line 19: debian8 \ nit: Usually we prefer spaces to tabs http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md File docker/README.md: http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md@18 PS1, Line 18: commited typo: committed http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md@26 PS1, Line 26: ubunu typo: ubuntu http://gerrit.cloudera.org:8080/#/c/12810/1/docker/README.md@35 PS1, Line 35: it is possible to specify urls in the DISTROS : make variable. Does this mean that specifying this URL would cause the Docker images to be fetched from the given registry (instead of building the containers)? -- To view, visit http://gerrit.cloudera.org:8080/12810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id971bda58d537fa15ec63004f744d3f730bad170 Gerrit-Change-Number: 12810 Gerrit-PatchSet: 1 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 20 Mar 2019 16:40:13 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 ) Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2480/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 16:40:05 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2479/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Mar 2019 16:23:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12797 to look at the new patch set (#5). Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. IMPALA-4356,IMPALA-7331: codegen all ScalarExprs Based on initial draft patch by Pooja Nilangekar. Codegen'd expressions can be executed in two ways - either by being called directly from a fully codegend function, or from interpreted code via a function pointer (previously ScalarFnCall::scalar_fn_wrapper_). This change moves the function pointer from ScalarFnCall to its base class ScalarExpr, so the full expr tree can be codegen'd, not just the ScalarFnCall subtrees. The key refactoring and improvements are: * ScalarExpr::Get*Val() switches between interpreted and the codegen'd function pointer code paths in an inline function, avoiding a virtual function call to ScalarFnCal::Get*Val(). * Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(), which calls a virtual function GetCodegenComputeFnImpl(). * ScalarFnCall's logic for deciding whether to interpret or codegen is better abstracted and exposed to ScalarExpr as IsInterpretable() and ShouldCodegen() methods. * The ScalarExpr::codegend_compute_fn_ function pointer is only populated for expressions that are "codegen entry points". These include the roots of expr trees and non-root expressions where the parent expression calls Get*Val() from the pseudo-codegend GetCodegendComputeFnWrapper(). * ScalarFnCall is always initialised for interpreted execution. Otherwise the function pointer is needed for non-root expressions, e.g. to support ScalarExprEvaluator::GetConstantVal(). * Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal is modified to use the StringVal memory layout to allow code sharing with StringVal. These fixes allowed simplification of IsNotEmptyPredicate codegen (from IMPALA-7657). IMPALA-7331 (CHAR codegen support functions) is also fixed because it was simpler to enable CHAR codegen within ScalarExpr than to carry forward the exiting CHAR workarounds from ScalarFnCall. The CHAR-specific codegen support required in the scalar expr subsystem is very limited. StringVal intermediates are used everywhere. Only SlotRef actually operates on the different tuple layout, and the required codegen support for SlotRef already exists for UDA intermediates anyway. Testing: * Ran exhaustive tests. Perf: * Ran a basic insert benchmark, which went from 10.1s to 7.6s create table foo stored as parquet as select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end from tpch30_parquet.lineitem; * Added perf regression test to tpcds-insert, similar to the manual benchmark. * Ran single-node TPC-H with large and small scale factors, to estimate impact on execution perf and query startup time, respectively. +--+---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +--+---+-++++ | TPCH(30) | parquet / none / none | 6.84| -0.18% | 4.49 | -0.31% | +--+---+-++++ +--+--+---++-++---++---++-++ | Workload | Query| File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval | +--+--+---++-++---++---++-++ | TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47| +4.18% | 1.29% | 0.88%| 5 | +4.12% | 2.31| 5.81 | | TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61| +4.33% | 2.18% | 2.15%| 5 | +3.91% | 1.73| 3.09 | | TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50%| 5 | +1.36% | 2.02| 3.94 | | TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65%| 5 | +0.88% | 0.29| 0.73 | | TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35| +1.12% | 1.64% | 1.11%| 5 | +0.80% | 1.15| 1.26 | | TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91| +1.13% | 7.68% | 5.37%| 5 | -0.34% | -0.29 | 0.27 | | TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56%| 5 | +0.28% | 0.29| 0.34
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 ) Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3927/ DRY_RUN=true -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 20 Mar 2019 15:57:00 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
..
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/conditional-functions-ir.cc
File be/src/exprs/conditional-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/conditional-functions-ir.cc@94
PS4, Line 94: type IfExpr::Get##type##Interpreted(ScalarExprEvaluator* eval,
const TupleRow* row) const { \
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/is-not-empty-predicate.h
File be/src/exprs/is-not-empty-predicate.h:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/is-not-empty-predicate.h@34
PS4, Line 34: virtual Status GetCodegendComputeFnImpl(LlvmCodeGen* codegen,
llvm::Function** fn) override;
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/scalar-fn-call.cc@129
PS4, Line 129: // For IR UDF, the loading of the Init() and CloseContext()
functions is deferred until
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.h
File be/src/exprs/valid-tuple-id.h:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.h@44
PS4, Line 44: virtual IntVal GetIntValInterpreted(ScalarExprEvaluator*, const
TupleRow*) const override;
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.cc
File be/src/exprs/valid-tuple-id.cc:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.cc@51
PS4, Line 51: IntVal
ValidTupleIdExpr::GetIntValInterpreted(ScalarExprEvaluator* eval, const
TupleRow* row) const {
line too long (101 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/tests/query_test/test_codegen.py
File tests/query_test/test_codegen.py:
http://gerrit.cloudera.org:8080/#/c/12797/4/tests/query_test/test_codegen.py@77
PS4, Line 77: ;
flake8: E703 statement ends with a semicolon
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Wed, 20 Mar 2019 15:57:37 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12797 Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs .. IMPALA-4356,IMPALA-7331: codegen all ScalarExprs Based on initial draft patch by Pooja Nilangekar. Codegen'd expressions can be executed in two ways - either by being called directly from a fully codegend function, or from interpreted code via a function pointer (previously ScalarFnCall::scalar_fn_wrapper_). This change moves the function pointer from ScalarFnCall to its base class ScalarExpr, so the full expr tree can be codegen'd, not just the ScalarFnCall subtrees. The key refactoring and improvements are: * ScalarExpr::Get*Val() switches between interpreted and the codegen'd function pointer code paths in an inline function, avoiding a virtual function call to ScalarFnCal::Get*Val(). * Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(), which calls a virtual function GetCodegenComputeFnImpl(). * ScalarFnCall's logic for deciding whether to interpret or codegen is better abstracted and exposed to ScalarExpr as IsInterpretable() and ShouldCodegen() methods. * The ScalarExpr::codegend_compute_fn_ function pointer is only populated for expressions that are "codgen entry points". The include the roots of expr trees and non-root expressions where the parent expression calls Get*Val() from the pseudo-codegend GetCodegendComputeFnWrapper(). * ScalarFnCall is always initialised for interpreted execution. Othe the function pointer is needed for non-root expressions, e.g. to support ScalarExprEvaluator::GetConstantVal(). * Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal is modified to use the StringVal memory layout to allow code sharing with StringVal. These fixes allowed simplification of IsNotEmptyPredicate codegen (from IMPALA-7657). IMPALA-7331 (CHAR codegen support functions) is also fixed because it was simpler to enable CHAR codegen within ScalarExpr than to carry forward the exiting CHAR workarounds from ScalarFnCall. The CHAR-specific codegen support required in the scalar expr subsystem is very limited. StringVal intermediates are used everywhere. Only SlotRef actually operates on the different tuple layout, and the required codegen support for SlotRef already exists for UDA intermediates anyway. Testing: * Ran exhaustive tests. Perf: * Ran a basic insert benchmark, which went from 10.1s to 7.6s create table foo stored as parquet as select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end from tpch30_parquet.lineitem; * Added perf regression test to tpcds-insert, similar to the manual benchmark. * Ran single-node TPC-H with large and small scale factors, to estimate impact on execution perf and query startup time, respectively. +--+---+-++++ | Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) | +--+---+-++++ | TPCH(30) | parquet / none / none | 6.84| -0.18% | 4.49 | -0.31% | +--+---+-++++ +--+--+---++-++---++---++-++ | Workload | Query| File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval | +--+--+---++-++---++---++-++ | TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47| +4.18% | 1.29% | 0.88%| 5 | +4.12% | 2.31| 5.81 | | TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61| +4.33% | 2.18% | 2.15%| 5 | +3.91% | 1.73| 3.09 | | TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50%| 5 | +1.36% | 2.02| 3.94 | | TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65%| 5 | +0.88% | 0.29| 0.73 | | TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35| +1.12% | 1.64% | 1.11%| 5 | +0.80% | 1.15| 1.26 | | TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91| +1.13% | 7.68% | 5.37%| 5 | -0.34% | -0.29 | 0.27 | | TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56%| 5 | +0.28% | 0.29| 0.34 | | TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72|
[native-toolchain-CR] Add documentation for docker-related toolchain builds
Hector Acosta has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12810 Change subject: Add documentation for docker-related toolchain builds .. Add documentation for docker-related toolchain builds This commit adds basic documentation with regard to docker-based toolchain builds. It breifly outlines how to build images and how to use them to build the toolchain. I also added the ability to fetch images from a docker registry. And fixed an 'undefined' variable problem with docker-based builds. Change-Id: Id971bda58d537fa15ec63004f744d3f730bad170 --- M Makefile A docker/README.md M functions.sh 3 files changed, 52 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/10/12810/1 -- To view, visit http://gerrit.cloudera.org:8080/12810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id971bda58d537fa15ec63004f744d3f730bad170 Gerrit-Change-Number: 12810 Gerrit-PatchSet: 1 Gerrit-Owner: Hector Acosta Gerrit-Reviewer: Laszlo Gaal Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-7917 (Part 3): Decouple Sentry from Impala
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12684 ) Change subject: IMPALA-7917 (Part 3): Decouple Sentry from Impala .. Patch Set 13: Rebased after https://gerrit.cloudera.org/c/12632 rebase. -- To view, visit http://gerrit.cloudera.org:8080/12684 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1a5d3e0a3e86ac2b0329b56247357fca93229dd0 Gerrit-Change-Number: 12684 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Austin Nobis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 20 Mar 2019 15:40:00 + Gerrit-HasComments: No
