David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15642 )
Change subject: IMPALA-9362: Upgrade sqlparse 0.1.19 -> 0.3.1 ...................................................................... Patch Set 6: (4 comments) Thanks for the comments. With regard to adhoc perf testing, I think it's not a bad idea. I think Thomas actually had a use case that he ran into recently that would be good to use. http://gerrit.cloudera.org:8080/#/c/15642/5/LICENSE.txt File LICENSE.txt: http://gerrit.cloudera.org:8080/#/c/15642/5/LICENSE.txt@616 PS5, Line 616: sqlparse-0.3.1: > version update? Done http://gerrit.cloudera.org:8080/#/c/15642/5/README.md File README.md: http://gerrit.cloudera.org:8080/#/c/15642/5/README.md@a75 PS5, Line 75: > why remove all of this? I actually think it's misleading. For one thing, which PYTHONPATH are we talking about? If you run impala-python, PYTHONPATH is different than when you run impala-shell.sh (once https://gerrit.cloudera.org/c/15524/ goes through). But even before then, when you look at set-pythonpath.sh, there's never a point where shell/ext-py/* is appended to PYTHONPATH. Those libs are solely for inclusion in the tarball deliverable. http://gerrit.cloudera.org:8080/#/c/15642/5/infra/python/deps/requirements.txt File infra/python/deps/requirements.txt: http://gerrit.cloudera.org:8080/#/c/15642/5/infra/python/deps/requirements.txt@a60 PS5, Line 60: > somewhat related question, why do we need to check in a copy of sqlparse? When we build the self-contained shell tarball that gets deployed to customers, we don't actually pull down any third-party libs from PyPI -- we use the packages checked in here: https://github.com/apache/impala/tree/master/shell/ext-py. sqlparse is just one of them. (I've gradually been updating the others too.) However, when we locally run impala-shell.sh, impala-python, or impala-py.test in the dev environment, we use the libs from infra/python/env/lib/*. (Caveat: when we use impala-py.test to run tests/shell/*, we do in fact test the shell environment from the deliverable that customers receive.) The script hat builds the tarball deliverable is https://github.com/apache/impala/blob/master/shell/make_shell_tarball.sh. Up until now, we have been using a patched version of 0.1.19 that was different than the upstream v0.1.19. That patch (from FredyW) was finally merged into upstream sqlparse in v0.3.0: https://github.com/andialbrecht/sqlparse/blob/master/CHANGELOG#L54 http://gerrit.cloudera.org:8080/#/c/15642/5/infra/python/deps/requirements.txt@a59 PS5, Line 59: : : > is this no longer true? Technically, this comment applies to any package in any of these *requirements.txt files that also happens to be in shell/ext-py, not just sqlparse. -- To view, visit http://gerrit.cloudera.org:8080/15642 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77a1fd5ae311634a18ee04b8c389d8a3f3a6e001 Gerrit-Change-Number: 15642 Gerrit-PatchSet: 6 Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 07 Apr 2020 17:34:59 +0000 Gerrit-HasComments: Yes
