You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/10/09 03:57:56 UTC

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var 

Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11623


Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................

IMPALA-7673: Support values from other variables in Impala shell --var

Prior to this patch, Impala shell --var could not accept values from
other variables unlike the one in Impala interactive shell with the SET
command.  This patch refactors the logic of variable substitution to
use the same logic in both interactive and command line shells.

Testing:
- Added a new shell test
- Ran all shell tests

Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
---
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
2 files changed, 59 insertions(+), 42 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/11623/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................

IMPALA-7673: Support values from other variables in Impala shell --var

Prior to this patch, Impala shell --var could not accept values from
other variables unlike the one in Impala interactive shell with the SET
command.  This patch refactors the logic of variable substitution to
use the same logic in both interactive and command line shells.

Example:
$ impala-shell.sh \
    --var="msg1=1" \
    --var="msg2=\${var:msg1}2" \
    --var="msg3=\${var:msg1}\${var:msg2}"

[localhost:21000] default> select ${var:msg3};
Query: select 112
+-----+
| 112 |
+-----+
| 112 |
+-----+

Testing:
- Added a new shell test
- Ran all shell tests

Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Reviewed-on: http://gerrit.cloudera.org:8080/11623
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
2 files changed, 73 insertions(+), 42 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py@1507
PS5, Line 1507: 
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py@1517
PS5, Line 1517: 
> flake8: E502 the backslash is redundant between brackets
Done


http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py@1534
PS5, Line 1534: 
> flake8: E302 expected 2 blank lines, found 1
Done


http://gerrit.cloudera.org:8080/#/c/11623/5/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/5/tests/shell/test_shell_commandline.py@736
PS5, Line 736:  
> flake8: F811 redefinition of unused 'test_var_substitution' from line 528
Done


http://gerrit.cloudera.org:8080/#/c/11623/5/tests/shell/test_shell_commandline.py@750
PS5, Line 750: 
> flake8: W292 no newline at end of file
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 04:03:32 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1028/ : 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/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 22:54:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 10: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:50:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/988/ : 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/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 04:40:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py@556
PS8, Line 556: query="select \'${var:msg3}
What will happen if impala shell is started without --query + some variables that reference non-existing variables? I think that the current logic will not result in an error, while it should.



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 08:36:38 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................

IMPALA-7673: Support values from other variables in Impala shell --var

Prior to this patch, Impala shell --var could not accept values from
other variables unlike the one in Impala interactive shell with the SET
command.  This patch refactors the logic of variable substitution to
use the same logic in both interactive and command line shells.

Example:
$ impala-shell.sh \
    --var="msg1=1" \
    --var="msg2=\${var:msg1}2" \
    --var="msg3=\${var:msg1}\${var:msg2}"

[localhost:21000] default> select ${var:msg3};
Query: select 112
+-----+
| 112 |
+-----+
| 112 |
+-----+

Testing:
- Added a new shell test
- Ran all shell tests

Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
---
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
2 files changed, 66 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/11623/8
-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 9: Code-Review+1

(1 comment)

Carry Csaba's +1.

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py@556
PS8, Line 556: query="select \'${var:msg3}
> Thanks for the explanation!
Done.



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 16:01:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py@556
PS8, Line 556: query="select \'${var:msg3}
> What will happen if impala shell is started without --query + some variable
It actually will.

Before the patch:

-- without --query
$ impala-shell.sh --var="foo=1"
[localhost:21000] default> select ${var:bar};
Error: Unknown variable BAR

-- with --query
$ impala-shell.sh --var="foo=1" --query="select \${var:bar}"
Error: Unknown variable BAR
Could not execute command: select ${var:bar}

-- referencing a variable that does not exist
-- this is wrong and the behavior is inconsistent with using SET
$ impala-shell.sh --var="foo=1" --var='bar=${var:baz}' --query='select "${var:bar}"'
Query: select "${var:baz}"
+--------------+
| '${var:baz}' |
+--------------+
| ${var:baz}   |
+--------------+
Fetched 1 row(s) in 0.11s

After the patch:

-- without --query
impala-shell.sh --var="foo=1"
[localhost:21000] default> select ${var:bar};
Error: Unknown variable BAR

-- with --query
$ impala-shell.sh --var="foo=1" --query="select \${var:bar}"
Error: Unknown variable BAR
Could not execute command: select ${var:bar}

-- referencing a variable that does not exist
-- this is correct and the behavior is consistent with SET
$ impala-shell.sh --var="foo=1" --var='bar=${var:baz}' --query='select "${var:bar}"'
Starting Impala Shell without Kerberos authentication
Error: Unknown variable BAZ



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Oct 2018 16:04:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 16:20:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 10: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 20:58:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 8: Code-Review+1

(1 comment)

I would prefer to add a few more asserts to the tests, lgtm otherwise.

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/8/tests/shell/test_shell_commandline.py@556
PS8, Line 556: query="select \'${var:msg3}
> It actually will.
Thanks for the explanation!
Can you add asserts that check the result for the error about missing BAZ + check that the select is not executed?



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 10:15:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................

IMPALA-7673: Support values from other variables in Impala shell --var

Prior to this patch, Impala shell --var could not accept values from
other variables unlike the one in Impala interactive shell with the SET
command.  This patch refactors the logic of variable substitution to
use the same logic in both interactive and command line shells.

Testing:
- Added a new shell test
- Ran all shell tests

Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
---
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
2 files changed, 60 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/11623/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3313/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 10
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 20:58:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py@1507
PS5, Line 1507: def replace_variables(set_variables, query):
flake8: E302 expected 2 blank lines, found 1


http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py@1517
PS5, Line 1517: \
flake8: E502 the backslash is redundant between brackets


http://gerrit.cloudera.org:8080/#/c/11623/5/shell/impala_shell.py@1534
PS5, Line 1534: def get_var_name(name):
flake8: E302 expected 2 blank lines, found 1


http://gerrit.cloudera.org:8080/#/c/11623/5/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/5/tests/shell/test_shell_commandline.py@736
PS5, Line 736: d
flake8: F811 redefinition of unused 'test_var_substitution' from line 528


http://gerrit.cloudera.org:8080/#/c/11623/5/tests/shell/test_shell_commandline.py@750
PS5, Line 750: 
flake8: W292 no newline at end of file



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 03:58:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11623/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11623/6//COMMIT_MSG@13
PS6, Line 13: 
Can you add a simple example? I only understood the purpose of the change after looking at the tests.


http://gerrit.cloudera.org:8080/#/c/11623/6/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/11623/6/shell/impala_shell.py@1508
PS6, Line 1508: def replace_variables(set_variables, query):
              :   """Replaces variable within the query text with their corresponding values using the
              :   given set_variables."""
'query' doesn't need to be a query anymore. Please update variable name and comment to reflect that it can be any kind of string.


http://gerrit.cloudera.org:8080/#/c/11623/6/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/6/tests/shell/test_shell_commandline.py@543
PS6, Line 543:     self._validate_shell_messages(result.stderr, ['112', 'Fetched 1 row(s)'],
Please add a test for the case when a not-yet-existing variable is referenced in a variable.



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 18:47:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/987/ : 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/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 04:33:31 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11623/6/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/11623/6/shell/impala_shell.py@1508
PS6, Line 1508: def replace_variables(set_variables, string):
              :   """Replaces variable within the string with their corresponding values using the
              :   given set_variables."""
> 'query' doesn't need to be a query anymore. Please update variable name and
Done


http://gerrit.cloudera.org:8080/#/c/11623/6/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/11623/6/tests/shell/test_shell_commandline.py@543
PS6, Line 543:     self._validate_shell_messages(result.stderr, ['112', 'Fetched 1 row(s)'],
> Please add a test for the case when a not-yet-existing variable is referenc
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Oct 2018 22:18:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7673: Support values from other variables in Impala shell --var

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/11623 )

Change subject: IMPALA-7673: Support values from other variables in Impala shell --var
......................................................................

IMPALA-7673: Support values from other variables in Impala shell --var

Prior to this patch, Impala shell --var could not accept values from
other variables unlike the one in Impala interactive shell with the SET
command.  This patch refactors the logic of variable substitution to
use the same logic in both interactive and command line shells.

Example:
$ impala-shell.sh \
    --var="msg1=1" \
    --var="msg2=\${var:msg1}2" \
    --var="msg3=\${var:msg1}\${var:msg2}"

[localhost:21000] default> select ${var:msg3};
Query: select 112
+-----+
| 112 |
+-----+
| 112 |
+-----+

Testing:
- Added a new shell test
- Ran all shell tests

Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
---
M shell/impala_shell.py
M tests/shell/test_shell_commandline.py
2 files changed, 73 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/23/11623/9
-- 
To view, visit http://gerrit.cloudera.org:8080/11623
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5b9fda329c45f2e5682f3cbc76d29ceca2e226a
Gerrit-Change-Number: 11623
Gerrit-PatchSet: 9
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>