You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2023/05/16 15:11:30 UTC

[Impala-ASF-CR] WIP: Optimize delimited output

Csaba Ringhofer has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19894


Change subject: WIP: Optimize delimited output
......................................................................

WIP: Optimize delimited output

Improvement for select * from tpch_parquet.lineitem:

python2 + hs2: 42s516ms -> 22s335ms
python2 + beeswax: 1m4s -> 22s126ms
python3 + hs2: 30s844ms -> 22s173ms
python3 + beeswax: 20s502ms -> 11s860ms

Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
---
M shell/shell_output.py
1 file changed, 62 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19894/1
-- 
To view, visit http://gerrit.cloudera.org:8080/19894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19894 )

Change subject: IMPALA-12171: Optimize delimited output
......................................................................


Patch Set 7:

Build Successful 

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Jun 2023 16:53:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/19894 )

Change subject: IMPALA-12171: Optimize delimited output
......................................................................


Patch Set 6:

(4 comments)

Thanks for doing this optimization!

http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py
File shell/shell_output.py:

http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py@105
PS6, Line 105:         or result.find(quote) != -1:
Currently we can't turn the try_quick_path flag back to True once it's turned off by error. It'd be nice to add an "unsafe" option for impala-shell to always (blindly) skip these checks. So we can know how well we can achieve in our best. On the other side, if users believe their strings are simple, they can use the "unsafe" option to futher boost the performance.


http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py@123
PS6, Line 123:         pass
nit: don't need 'pass'


http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py@149
PS6, Line 149:         self.try_quick_path = False
For supportability, we need to know when (and wound be perfect to have "why") this is turned off. Or just print it if verbose=true.


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

http://gerrit.cloudera.org:8080/#/c/19894/6/tests/shell/test_shell_commandline.py@1628
PS6, Line 1628: 
Can we add tests for decoding errors? We can use the queries in test_utf8_decoding_error_handling(), e.g.

  select unhex('aa')



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Mon, 05 Jun 2023 09:14:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19894 )

Change subject: IMPALA-12171: Optimize delimited output
......................................................................


Patch Set 8:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/19894/6//COMMIT_MSG@16
PS6, Line 16: python2 + hs2: 42s516ms -> 22s335ms
> Optional: you could also include a percentage of performance improvement, i
I didn't want to add percentages as I cannot say something like "ClientFetchWaitTimer was improved 50% for Python3 + HS2" - this really depends on other factors like data types and number of columns/rows. This specific benchmarks show s that the improvement is significant, but I wouldn't want to extrapolate to other queries.


http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py
File shell/shell_output.py:

http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py@102
PS6, Line 102: [
> Optional: You could use () instead of [], which would create a lazy iterato
Stayed with [] as I do not know lazy iterators that well. I tested it and the speed didn't change.


http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py@105
PS6, Line 105:         or result.find(quote) != -1:
> Currently we can't turn the try_quick_path flag back to True once it's turn
I wouldn't complicate this part as I am considering actually implementing quoting in the future to handle all cases. The quoting rules are not too complex but would need robust testing to ensure that we always return the same results as Python's csv module.

The find/count functions add minimal overhead, the only one that is somewhat significant is decoding.

Note that DelimitedOutputFormatter is created per query, so if there are several queries the shell will retry the quick path in each query.


http://gerrit.cloudera.org:8080/#/c/19894/6/shell/shell_output.py@123
PS6, Line 123: 
> nit: don't need 'pass'
Done


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

http://gerrit.cloudera.org:8080/#/c/19894/6/tests/shell/test_shell_commandline.py@33
PS6, Line 33: try:
> Could we check the python version and branch on that instead of try-except?
This was copied from shell_output.py and I am not sure that the comments are always correct (is cStringIO always available on Python2?).


http://gerrit.cloudera.org:8080/#/c/19894/6/tests/shell/test_shell_commandline.py@1597
PS6, Line 1597:  
> Nit: result sets.
Done


http://gerrit.cloudera.org:8080/#/c/19894/6/tests/shell/test_shell_commandline.py@1599
PS6, Line 1599: .
> Nit: . (full stop).
Done


http://gerrit.cloudera.org:8080/#/c/19894/6/tests/shell/test_shell_commandline.py@1628
PS6, Line 1628: 
> Can we add tests for decoding errors? We can use the queries in test_utf8_d
Added a test and bumped into an issue with Hive: HIVE-27418 (and learned that with strict hs2 protocol the shell tests connect to Hive).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Jun 2023 16:51:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Hello Quanlong Huang, Daniel Becker, Jason Fehr, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19894

to look at the new patch set (#8).

Change subject: IMPALA-12171: Optimize delimited output
......................................................................

IMPALA-12171: Optimize delimited output

The change adds a CSV generator that can handle simple cases
(no characters that need qouting) and falls back to Python's
builtin csv module if special characters are found.

Improvement of ClientFetchWaitTimer for
select * from tpch_parquet.lineitem:

python2 + hs2: 42s516ms -> 22s335ms
python2 + beeswax: 1m4s -> 22s126ms
python3 + hs2: 30s844ms -> 22s173ms
python3 + beeswax: 20s502ms -> 11s860ms

The different amount of improvement per protocol/Python version
probably comes from the varying amount utf-8 conversions that the
patch avoids. It seems that doing the conversion for the large
string after concatenation is much faster than doing it for a
lot of small strings.

Testing:
- added some shell tests with special characters and ran them

Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
---
M shell/shell_output.py
M tests/shell/test_shell_commandline.py
2 files changed, 123 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 8
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Hello Jason Fehr, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19894

to look at the new patch set (#6).

Change subject: IMPALA-12171: Optimize delimited output
......................................................................

IMPALA-12171: Optimize delimited output

The change adds a CSV generator that can handle simple cases
(no characters that need qouting) and falls back to Python's
builtin csv module if special characters are found.

Improvement of ClientFetchWaitTimer for
select * from tpch_parquet.lineitem:

python2 + hs2: 42s516ms -> 22s335ms
python2 + beeswax: 1m4s -> 22s126ms
python3 + hs2: 30s844ms -> 22s173ms
python3 + beeswax: 20s502ms -> 11s860ms

The different amount of improvement per protocol/Python version
probably comes from the varying amount utf-8 conversions that the
patch avoids. It seems that doing the conversion for the large
string after concatenation is much faster than doing it for a
lot of small strings.

Testing:
- added some shell tests with special characters and ran them

Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
---
M shell/shell_output.py
M tests/shell/test_shell_commandline.py
2 files changed, 114 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19894 )

Change subject: IMPALA-12171: Optimize delimited output
......................................................................


Patch Set 9:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/13246/ : Initial code review checks failed. See linked job for details on the failure.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 9
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Jun 2023 18:06:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-12171: Optimize delimited output

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.

Hello Quanlong Huang, Daniel Becker, Jason Fehr, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/19894

to look at the new patch set (#7).

Change subject: IMPALA-12171: Optimize delimited output
......................................................................

IMPALA-12171: Optimize delimited output

The change adds a CSV generator that can handle simple cases
(no characters that need qouting) and falls back to Python's
builtin csv module if special characters are found.

Improvement of ClientFetchWaitTimer for
select * from tpch_parquet.lineitem:

python2 + hs2: 42s516ms -> 22s335ms
python2 + beeswax: 1m4s -> 22s126ms
python3 + hs2: 30s844ms -> 22s173ms
python3 + beeswax: 20s502ms -> 11s860ms

The different amount of improvement per protocol/Python version
probably comes from the varying amount utf-8 conversions that the
patch avoids. It seems that doing the conversion for the large
string after concatenation is much faster than doing it for a
lot of small strings.

Testing:
- added some shell tests with special characters and ran them

Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
---
M shell/shell_output.py
M tests/shell/test_shell_commandline.py
M tests/shell/util.py
3 files changed, 129 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/19894/7
-- 
To view, visit http://gerrit.cloudera.org:8080/19894
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I671c6f538c588f8ad4ef4067f7bc8a6b8a5220cb
Gerrit-Change-Number: 19894
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>