[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12823/5/tests/shell/good_impalarc File tests/shell/good_impalarc: PS5: > If this breaks existing .impalarc files, I'd prefer to wait until 4.0. I can make this backward compatible and still support the new feature, too. I'll do that in my next PS. -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Comment-Date: Wed, 27 Mar 2019 16:47:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/12823/5/tests/shell/good_impalarc File tests/shell/good_impalarc: PS5: If this breaks existing .impalarc files, I'd prefer to wait until 4.0. -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Comment-Date: Wed, 27 Mar 2019 03:26:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 5: Code-Review+1 (1 comment) Other reviewers probably have more comments. So +1 ing this. LGTM. http://gerrit.cloudera.org:8080/#/c/12823/5/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/12823/5/shell/option_parser.py@132 PS5, Line 132: config = ConfigParser.ConfigParser() Do we need this? we convert it to uppercase anyway (L144) -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 26 Mar 2019 23:54:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2539/ : 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/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 26 Mar 2019 15:13:26 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@10 PS2, Line 10: both short and long flag names instead of optparse's dest names > My life would be easier if you explained what a 'dest name' is here. Done http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@24 PS2, Line 24: Q=DEFAULT_FILE_FORMAT=parquet > Flags can be repeated. So if I have Impala shell allows it, e.g. impala-shell.sh -query_option=DEFAULT=parquet -Q DEFAULT_FILE_FORMAT=orc The behavior is undefined, it can either be parquet or orc. -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 26 Mar 2019 14:40:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. IMPALA-8330: Impala shell config file should use flag names This patch changes the file format in Impala shell config file to accept both short and long flag names instead of optparse's dest names (variable names to store flag values) because dest names are internal to Impala shell. This patch does not change the format and behavior of Impala shell config file with [impala.query_options] section. Format: [impala] flag_name=flag_value Example: [impala] ; This is long flag. query=select 1 ; This is short flag. Q=DEFAULT_FILE_FORMAT=parquet ; Flags can be repeated with , var=msg1=hello,var=msg2=world Testing: - Ran all E2E shell tests on Python 2.6 and 2.7. Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 --- M shell/impala_shell.py M shell/option_parser.py M tests/shell/good_impalarc 3 files changed, 31 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/12823/5 -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 2: (2 comments) Looks good http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@10 PS2, Line 10: both short and long flag names instead of dest names because dest names My life would be easier if you explained what a 'dest name' is here. http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@24 PS2, Line 24: ; Flags can be repeated. Flags can be repeated. So if I have query_options=DEFAULT_FILE_FORMAT=parquet Q=DEFAULT_FILE_FORMAT=orc will it all work OK? -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 25 Mar 2019 18:51:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. IMPALA-8330: Impala shell config file should use flag names This patch changes the file format in Impala shell config file to accept both short and long flag names instead of dest names because dest names are internal to Impala shell. This patch does not change the format and behavior of Impala shell config file with [impala.query_options] section. Format: [impala] flag_name=flag_value Example: [impala] ; This is long flag. query=select 1 ; This is short flag. Q=DEFAULT_FILE_FORMAT=parquet ; Flags can be repeated. var=msg1=hello var=msg2=world Testing: - Ran all E2E shell tests Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 --- M shell/impala_shell.py M shell/option_parser.py M tests/shell/good_impalarc 3 files changed, 31 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/12823/3 -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2518/ : 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/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 22 Mar 2019 00:35:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@67 PS2, Line 67: maps short and long option name to option for a qui > also mention it maps to the dest? Done. it actually maps to option. Option can have attributes like "action", "dest", etc. http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@275 PS2, Line 275: # The code below removes the - from the short option and -- from the long option. > I had to print these stuff locally to understand whats going on here. Add a Done http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@277 PS2, Line 277: long_opt = option._long_opts[0][2:] if len(option._long_opts) > 0 else None > I think this part is subtle. It looks like we are updating defaults to also Done -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 22 Mar 2019 00:18:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@23 PS2, Line 23: Q > It's just an additional feature that we support. I agree that users should Okay. For a second, I forgot that the options parser supported short opts. So I got confused. http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py File shell/option_parser.py: http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@67 PS2, Line 67: contains short and long options for a quick lookup. also mention it maps to the dest? http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@275 PS2, Line 275: short_opt = option._short_opts[0][1:] if len(option._short_opts) > 0 else None I had to print these stuff locally to understand whats going on here. Add an example may be? (that it returns ['-f'] and ['--foo']... http://gerrit.cloudera.org:8080/#/c/12823/2/shell/option_parser.py@277 PS2, Line 277: if short_opt in defaults: I think this part is subtle. It looks like we are updating defaults to also map from dest_name to default_val... May be add a line or two about it? -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 22:06:43 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@23 PS2, Line 23: Q > Honestly, I'm not a super big fan of this short naming. I think we'd unnece It's just an additional feature that we support. I agree that users should use long flag instead. The problem is if we ever have a flag without long name then it will become the limitation of the config file. -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 19:11:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12823/2//COMMIT_MSG@23 PS2, Line 23: Q Honestly, I'm not a super big fan of this short naming. I think we'd unnecessarily make this confusing. Thoughts? -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 19:06:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12823 ) Change subject: IMPALA-8330: Impala shell config file should use flag names .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2502/ : 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/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 21 Mar 2019 18:00:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8330: Impala shell config file should use flag names
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12823 Change subject: IMPALA-8330: Impala shell config file should use flag names .. IMPALA-8330: Impala shell config file should use flag names This patch changes the file format in Impala shell config file to accept both short and long flag names instead of dest names because dest names are internal to Impala shell. This patch does not change the format and behavior of Impala shell config file with [impala.query_options] section. Format: [impala] flag_name=flag_value Example: [impala] ; This is long flag. query=select 1 ; This is short flag. Q=DEFAULT_FILE_FORMAT=parquet ; Flags can be repeated. var=msg1=hello var=msg2=world Testing: - Ran all E2E shell tests Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 --- M shell/impala_shell.py M shell/option_parser.py M tests/shell/good_impalarc 3 files changed, 27 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/12823/2 -- To view, visit http://gerrit.cloudera.org:8080/12823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic43603c1b538af08fddcab1b2c1f6ad1af1a6cb9 Gerrit-Change-Number: 12823 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya
