You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Brown (Code Review)" <ge...@cloudera.org> on 2016/10/27 21:17:31 UTC
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Michael Brown has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4873
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Test infrastructure, including the random query generator and the data
migrator, needs to know the primary keys of Impala/Kudu tables. This
test infrastructure keeps Python object models of the tables and
columns. This patch adds the ability to read from source Impala/Kudu
tables via SHOW CREATE TABLE and store primary keys as proper
attributes. The patch also adds tests that ensure the test
infrastructure is always able to read and store the primary keys. This
helps find breakages sooner rather than later. For example, if a
regression to "SHOW CREATE TABLE" or the test infrastructure makes us no
longer able to parse primary keys, GVO or other CI will find the
breakage faster than running the query generator.
I also fixed some flake8 issues in files I touched. There were several
files that had a lot of white space warnings, and I wanted to keep the
patch from getting too large.
Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/conftest.py
M tests/metadata/test_show_create_table.py
4 files changed, 151 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/4873/1
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 7:
Thanks for the review, Taras. This *does* need GVO, which I'll make sure to do.
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 2:
(1 comment)
Thanks for the comment, Taras. Please set patch set 2.
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
Line 407: table.add_col(col)
> col.is_primary_key = col_name in primary_key_names
Done
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 3:
(4 comments)
Thanks Matthew. Please see patch set 3.
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/comparison/common.py
File tests/comparison/common.py:
PS2, Line 527: updatable_columns
> it might be more helpful to use an existing concept, e.g. 'non_pk_cols' or
I don't agree. I think a name like "non_pk_cols" can end up leading to confusion. What should "non_pk_cols" do if the Table is, say, an Impala TEXTTABLE, or a Postgres table based off a TEXTTABLE? Should it return all of the columns, because none are primary keys, or should it return none of the columns, because that table doesn't have any primary keys? Someone then has to go read the docstring to know. Meanwhile the property is needed for the query generator to know what keys it may update.
So I think updatable_columns is a more accurate name.
Can you comment, or mark Done if this is OK?
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
PS2, Line 537: is
> ?
Done
PS2, Line 695: EATE TABLE db.table (
> can you comment if this is the same format printed for 1 PK col as well?
Done
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/conftest.py
File tests/conftest.py:
Line 456: generator, stress test, etc.
> but why is this necessary?
Done
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Test infrastructure, including the random query generator and the data
migrator, needs to know the primary keys of Impala/Kudu tables. This
test infrastructure keeps Python object models of the tables and
columns. This patch adds the ability to read from source Impala/Kudu
tables via SHOW CREATE TABLE and store primary keys as proper
attributes. The patch also adds tests that ensure the test
infrastructure is always able to read and store the primary keys. This
helps find breakages sooner rather than later. For example, if a
regression to "SHOW CREATE TABLE" or the test infrastructure makes us no
longer able to parse primary keys, GVO or other CI will find the
breakage faster than running the query generator.
I also fixed some flake8 issues in files I touched. There were several
files that had a lot of white space warnings, and I wanted to keep the
patch from getting too large.
Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Reviewed-on: http://gerrit.cloudera.org:8080/4873
Reviewed-by: Michael Brown <mi...@cloudera.com>
Reviewed-by: Taras Bobrovytsky <tb...@cloudera.com>
Tested-by: Internal Jenkins
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/conftest.py
M tests/metadata/test_show_create_table.py
4 files changed, 175 insertions(+), 29 deletions(-)
Approvals:
Michael Brown: Looks good to me, but someone else must approve
Taras Bobrovytsky: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.
David Knupp has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
PS1, Line 538: return ()
What am I missing here? This doesn't do what the docstring claims -- it only returns an empty tuple.
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/metadata/test_show_create_table.py
File tests/metadata/test_show_create_table.py:
PS1, Line 241: TestInfraCompat
I have wondered whether we should create a new directory under ${IMPALA_HOME}/tests called "infra", as a place to keep (an hopefully add) tests of the infrastructure itself. What do you think of that idea?
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has uploaded a new patch set (#4).
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Test infrastructure, including the random query generator and the data
migrator, needs to know the primary keys of Impala/Kudu tables. This
test infrastructure keeps Python object models of the tables and
columns. This patch adds the ability to read from source Impala/Kudu
tables via SHOW CREATE TABLE and store primary keys as proper
attributes. The patch also adds tests that ensure the test
infrastructure is always able to read and store the primary keys. This
helps find breakages sooner rather than later. For example, if a
regression to "SHOW CREATE TABLE" or the test infrastructure makes us no
longer able to parse primary keys, GVO or other CI will find the
breakage faster than running the query generator.
I also fixed some flake8 issues in files I touched. There were several
files that had a lot of white space warnings, and I wanted to keep the
patch from getting too large.
Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/conftest.py
M tests/metadata/test_show_create_table.py
4 files changed, 175 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/4873/4
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4873/5/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
PS5, Line 532: _get
Maybe it would be better to call this something other than get? Maybe 'load' or 'fetch'.
To me get sounds like it's returning some value in a local varaible.
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/common.py
File tests/comparison/common.py:
Line 524: return tuple(col for col in self._cols if col.is_primary_key)
> In general I believe primary keys to be ordered, and that the order is inde
It turns out the ordering is based off the declaration ordering. See IMPALA-4402
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 5: Code-Review+1
rebase
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
Line 407: col.is_primary_key = True
col.is_primary_key = col_name in primary_key_names
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Harrison Sheinblatt (Code Review)" <ge...@cloudera.org>.
Harrison Sheinblatt has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/common.py
File tests/comparison/common.py:
Line 524: return tuple(col for col in self._cols if col.is_primary_key)
In general I believe primary keys to be ordered, and that the order is independent of declaration order of columns in the create table statement. I think it would be better to store the primary keys as a separate list like _cols to maintain that order information in the Table class.
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has uploaded a new patch set (#3).
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Test infrastructure, including the random query generator and the data
migrator, needs to know the primary keys of Impala/Kudu tables. This
test infrastructure keeps Python object models of the tables and
columns. This patch adds the ability to read from source Impala/Kudu
tables via SHOW CREATE TABLE and store primary keys as proper
attributes. The patch also adds tests that ensure the test
infrastructure is always able to read and store the primary keys. This
helps find breakages sooner rather than later. For example, if a
regression to "SHOW CREATE TABLE" or the test infrastructure makes us no
longer able to parse primary keys, GVO or other CI will find the
breakage faster than running the query generator.
I also fixed some flake8 issues in files I touched. There were several
files that had a lot of white space warnings, and I wanted to keep the
patch from getting too large.
Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/conftest.py
M tests/metadata/test_show_create_table.py
4 files changed, 166 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/4873/3
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 6: Code-Review+1
rebase
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/comparison/common.py
File tests/comparison/common.py:
PS2, Line 527: updatable_columns
> I don't agree. I think a name like "non_pk_cols" can end up leading to conf
Sure, I don't feel strongly, but I don't think of it being weird for other table types -- they really have no notion of a primary key, so all cols are not PK cols. It's like saying we can't ask if a col is decimal or not just because a particular storage format doesn't support decimal. Further, it is common to reason about the abstraction of a "Table" as something having PKs or not, whereas 'updatable' isn't a property that is common on this abstraction.
Anyway, that's just what I was thinking. I'm fine either way.
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/comparison/common.py
File tests/comparison/common.py:
PS2, Line 527: updatable_columns
it might be more helpful to use an existing concept, e.g. 'non_pk_cols' or something like that
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
PS2, Line 537: ()
?
PS2, Line 695: PRIMARY KEY (pk1, pk2)
can you comment if this is the same format printed for 1 PK col as well?
http://gerrit.cloudera.org:8080/#/c/4873/2/tests/conftest.py
File tests/conftest.py:
Line 456: generator, stress test, etc.
but why is this necessary?
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4873
to look at the new patch set (#7).
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Test infrastructure, including the random query generator and the data
migrator, needs to know the primary keys of Impala/Kudu tables. This
test infrastructure keeps Python object models of the tables and
columns. This patch adds the ability to read from source Impala/Kudu
tables via SHOW CREATE TABLE and store primary keys as proper
attributes. The patch also adds tests that ensure the test
infrastructure is always able to read and store the primary keys. This
helps find breakages sooner rather than later. For example, if a
regression to "SHOW CREATE TABLE" or the test infrastructure makes us no
longer able to parse primary keys, GVO or other CI will find the
breakage faster than running the query generator.
I also fixed some flake8 issues in files I touched. There were several
files that had a lot of white space warnings, and I wanted to keep the
patch from getting too large.
Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/conftest.py
M tests/metadata/test_show_create_table.py
4 files changed, 175 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/4873/7
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 4:
Patch set 4 adds Table object properties for the names of the columns in addition to what we had before, which were full Column objects. I find myself checking the name, so this would be convenient to have. It also makes 1 of the tests a bit cleaner.
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 1:
(3 comments)
Thanks for the reviews.
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/common.py
File tests/comparison/common.py:
Line 524: return tuple(col for col in self._cols if col.is_primary_key)
> In general I believe primary keys to be ordered, and that the order is inde
I'm following up offline with Dimitris about this. How I fix this depends on the answer.
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
PS1, Line 538: return ()
> What am I missing here? This doesn't do what the docstring claims -- it onl
This is the base class method. We're only implementing the parsing for Impala tables at the moment. Note the override further below. Also, "or an empty tuple if there are no primary keys".
http://gerrit.cloudera.org:8080/#/c/4873/1/tests/metadata/test_show_create_table.py
File tests/metadata/test_show_create_table.py:
PS1, Line 241: TestInfraCompat
> I have wondered whether we should create a new directory under ${IMPALA_HOM
The tests in this case are very close to the edge of test infrastructure meeting with product. We need these tests to run continuously to catch changes in SHOW CREATE TABLE output. This differs from the tests in tests/comparison/tests, which don't really depend on changes in the product. So, I put these tests into a location that would hit GVO and CI builds.
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 7: Code-Review+1
(1 comment)
Thanks for the review. I made the rename and ran the tests locally.
http://gerrit.cloudera.org:8080/#/c/4873/5/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:
PS5, Line 532: _fet
> Maybe it would be better to call this something other than get? Maybe 'load
Done
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has uploaded a new patch set (#2).
Change subject: IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
......................................................................
IMPALA-4352: test infra: store Impala/Kudu primary keys in object model
Test infrastructure, including the random query generator and the data
migrator, needs to know the primary keys of Impala/Kudu tables. This
test infrastructure keeps Python object models of the tables and
columns. This patch adds the ability to read from source Impala/Kudu
tables via SHOW CREATE TABLE and store primary keys as proper
attributes. The patch also adds tests that ensure the test
infrastructure is always able to read and store the primary keys. This
helps find breakages sooner rather than later. For example, if a
regression to "SHOW CREATE TABLE" or the test infrastructure makes us no
longer able to parse primary keys, GVO or other CI will find the
breakage faster than running the query generator.
I also fixed some flake8 issues in files I touched. There were several
files that had a lot of white space warnings, and I wanted to keep the
patch from getting too large.
Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/conftest.py
M tests/metadata/test_show_create_table.py
4 files changed, 151 insertions(+), 29 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/4873/2
--
To view, visit http://gerrit.cloudera.org:8080/4873
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib654b6cd0e8c2a172ffb7330497be4d4a751e6e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Harrison Sheinblatt <hs...@hotmail.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>