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

Posted to dev@impala.apache.org by "Michael Brown (Code Review)" <ge...@cloudera.org> on 2016/08/22 19:21:59 UTC

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Michael Brown has uploaded a new change for review.

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

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................

IMPALA-4001: qgen: add proof of concept tests for Query() objects

This patch adds a simple proof-of-concept test framework, and a few
tests, for the random query generator, specifically the portion of the
random query generator that is responsible for taking a Query object and
doing something with it. The two pieces of functionality I chose for
exhibition are

1. Writing the query into Impala SQL
2. Reporting characteristics in the SELECT clause (used internally)

In the interest of keeping the patch small, I have not added many tests,
nor have I chose to focus on more areas for test. On its own this is
fairly simple. As I add features to this portion of the query generator,
though, it will be more useful to test new functionality and also
regression test the framework.

Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
---
A tests/comparison/tests/README
A tests/comparison/tests/fake_query.py
A tests/comparison/tests/query_object_testdata.py
A tests/comparison/tests/test_query_objects.py
4 files changed, 335 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mi...@cloudera.com>

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 4: Code-Review+1

rebase, carry +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.

David Knupp has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/fake_query.py
File tests/comparison/tests/fake_query.py:

PS1, Line 97: must at least a
"must at least *contain* a" ...?
"must at least *be* a" ...?


PS1, Line 97: fram
s/fram/from/


http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

PS1, Line 64: DATASET
So -- these are really test cases, yes? Could this just be called QUERY_TEST_CASES, or something. DATASET seems confusing, at least to me.


http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/test_query_objects.py
File tests/comparison/tests/test_query_objects.py:

PS1, Line 25: query_something
I'm a little confused by this. query_something is a ... QueryTest object (namedtuple), right? From the DATASET list? Maybe a more descriptive variable name could be employed here, to help clarify some of the pytest magic going on.


Line 53: @pytest.mark.parametrize('query_test', DATASET, ids=_idfn)
How do you feel about moving all of the tests to the end of the file, rather than interspersing them with utility functions?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Hello David Knupp,

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

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

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

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................

IMPALA-4001: qgen: add proof of concept tests for Query() objects

This patch adds a simple proof-of-concept test framework, and a few
tests, for the random query generator, specifically the portion of the
random query generator that is responsible for taking a Query object and
doing something with it. The two pieces of functionality I chose for
exhibition are

1. Writing the query into Impala SQL
2. Reporting characteristics in the SELECT clause (used internally)

In the interest of keeping the patch small, I have not added many tests,
nor have I chose to focus on more areas for test. On its own this is
fairly simple. As I add features to this portion of the query generator,
though, it will be more useful to test new functionality and also
regression test the framework.

Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
---
A tests/comparison/tests/README
A tests/comparison/tests/fake_query.py
A tests/comparison/tests/query_object_testdata.py
A tests/comparison/tests/test_query_objects.py
4 files changed, 335 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/4081/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4081
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/fake_query.py
File tests/comparison/tests/fake_query.py:

PS1, Line 97: must at least a
> "must at least *contain* a" ...?
Done


PS1, Line 97: fram
> s/fram/from/
Done


http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

PS1, Line 64: DATASET
> So -- these are really test cases, yes? Could this just be called QUERY_TES
Done


http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/test_query_objects.py
File tests/comparison/tests/test_query_objects.py:

PS1, Line 25: query_something
> I'm a little confused by this. query_something is a ... QueryTest object (n
Done


Line 53: @pytest.mark.parametrize('query_test', DATASET, ids=_idfn)
> How do you feel about moving all of the tests to the end of the file, rathe
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................

Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/fake_query.py
File tests/comparison/tests/fake_query.py:

PS4, Line 83: Return a Query object constructed by the keyword args above. select_clause and
: from_clause are required.
: """
> I could, but I still need something here that enforces the SELECT and FROM
Why are there different restrictions? I guess the query generator could generate queries without from clauses, but all queries should have a select clause, right? I'd think that should always be invalid.
Anyway, I'm fine with it if you need it.

http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

PS4, Line 26: namedtuple(
> It's a better way to specify a data container with meaningful member names.
Done

Sounds good. I hadn't seen it before.

PS4, Line 106: FakeSelectClause(
: FakeFirstValue(
> So far the Fake* aren't actually classes. They are functions that abstract
Ok, well I'm not trying to slow things down but something to consider for the future. It could get cumbersome if a lot of helper fns build up over time.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.

David Knupp has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4081/2/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

Line 70:         testid='select col from table',
A minor followup thought -- it might be nice to have testid as the first element in each test case. Making testid more prominent makes it more obvious at a glance what each test case is.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 4:

(3 comments)

Looks very useful, have a few questions though.

http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/fake_query.py
File tests/comparison/tests/fake_query.py:

PS4, Line 83:   Return a Query object constructed by the keyword args above. select_clause and
            :   from_clause are required.
            :   """
Why not change Query() to support keyword args to initialize?


http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

PS4, Line 26: namedtuple(
This might be obvious for other folks, but I had to google to determine that this was just creating a class called QueryTest with the following properties. Why not just define this as a class? If you keep it like this, consider adding a comment above this.


PS4, Line 106: FakeSelectClause(
             :                 FakeFirstValue(
I wonder if we could make the query objects easier to construct, e.g. making a builder patter a first class citizen of the model classes. If I understand this correctly, my concern is that we may have to create Fake* classes mirroring the underlying classes as we want to add new test cases.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

Posted by "David Knupp (Code Review)" <ge...@cloudera.org>.

David Knupp has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has uploaded a new patch set (#2).

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................

IMPALA-4001: qgen: add proof of concept tests for Query() objects

This patch adds a simple proof-of-concept test framework, and a few
tests, for the random query generator, specifically the portion of the
random query generator that is responsible for taking a Query object and
doing something with it. The two pieces of functionality I chose for
exhibition are

1. Writing the query into Impala SQL
2. Reporting characteristics in the SELECT clause (used internally)

In the interest of keeping the patch small, I have not added many tests,
nor have I chose to focus on more areas for test. On its own this is
fairly simple. As I add features to this portion of the query generator,
though, it will be more useful to test new functionality and also
regression test the framework.

Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
---
A tests/comparison/tests/README
A tests/comparison/tests/fake_query.py
A tests/comparison/tests/query_object_testdata.py
A tests/comparison/tests/test_query_objects.py
4 files changed, 335 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/4081/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4081
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................

Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/fake_query.py
File tests/comparison/tests/fake_query.py:

PS4, Line 83: Return a Query object constructed by the keyword args above. select_clause and
: from_clause are required.
: """
> Why are there different restrictions? I guess the query generator could gen
Leaving as is.

The reason is that when the query generator runs, a sparse Query() object gets instantiated, built up over time, inspected, and added to over the course of building the query. What I need here are already fully formed. So the restriction is put in place for unit tests, not the flow of generating a random query.

This might be a good way to identify places in code that demonstrate what I'm talking about:

$ git grep -En "( query\.[a-z_]+ = |query = Query|\.current_query)" -- tests/comparison/query_generator.py

If we enforced the restriction you speak of on Query() objects, we have to really refactor the query generator. Without automated tests, I'd rather not do that. Otherwise we risk some regression that's difficult to detect without collecting stats to validate our query profiles (planned in IMPALA-3993).

http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

PS4, Line 106: FakeSelectClause(
: FakeFirstValue(
> Ok, well I'm not trying to slow things down but something to consider for t
Thanks. I consider the wording of the TODO in fake_query.py a good note for this.

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

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4081/2/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

Line 70:         testid='select col from table',
> A minor followup thought -- it might be nice to have testid as the first el
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 4: Code-Review+1

carry +1 again, I guess. Gerrit is annoying.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 4: -Code-Review

(3 comments)

Matthew, thanks for the review. Please see comments. I'd like responses before I work on the next patch set.

http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/fake_query.py
File tests/comparison/tests/fake_query.py:

PS4, Line 83:   Return a Query object constructed by the keyword args above. select_clause and
            :   from_clause are required.
            :   """
> Why not change Query() to support keyword args to initialize?
I could, but I still need something here that enforces the SELECT and FROM clause attributes being set when building full Queries for unit tests. I don't want that enforcement for building Queries with the query generator. It didn't seem worth changing the actual Query() object for that purpose.

L18-33 tries generally to explain this, but maybe specific aspects of Query aren't explained as well as I could.

Would you like a better explanation in a comment?


http://gerrit.cloudera.org:8080/#/c/4081/4/tests/comparison/tests/query_object_testdata.py
File tests/comparison/tests/query_object_testdata.py:

PS4, Line 26: namedtuple(
> This might be obvious for other folks, but I had to google to determine tha
It's a better way to specify a data container with meaningful member names. It's closer to a struct, where you can't add fields.

1. Raw dictionaries and tuples aren't particularly helpful. You have to read code to understand what they're doing.

2. The attribute set is immutable, unlike in a class. This prevents the bad habit allowed by classes whereby attributes may be added or removed by a caller, changing the class's interface on the fly. In fact the query generator is full of these. It's because you have to read the code that sets the attributes to know what they do, as opposed to reading the class definition. So we get to use the features of the standard library to enforce rules.

3. But you get the syntactic sugar of attribute names to set and get data, instead of just positional orientation as in regular tuples.

The middle bits of the top answer here are good:

http://stackoverflow.com/questions/2970608/what-are-named-tuples-in-python#2970722

If this answer satisfies you, can you click Done? If not, let me know and I'll change to a class implementation if you wish, or add a comment. However, should all future namedtuples have a comment?


PS4, Line 106: FakeSelectClause(
             :                 FakeFirstValue(
> I wonder if we could make the query objects easier to construct, e.g. makin
So far the Fake* aren't actually classes. They are functions that abstract away the way objects get created and allow for a nested way to build full Query() objects. Note that for some, like FromClause we don't need them. However, the builder pattern is an intriguing idea.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

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

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

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

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


IMPALA-4001: qgen: add proof of concept tests for Query() objects

This patch adds a simple proof-of-concept test framework, and a few
tests, for the random query generator, specifically the portion of the
random query generator that is responsible for taking a Query object and
doing something with it. The two pieces of functionality I chose for
exhibition are

1. Writing the query into Impala SQL
2. Reporting characteristics in the SELECT clause (used internally)

In the interest of keeping the patch small, I have not added many tests,
nor have I chose to focus on more areas for test. On its own this is
fairly simple. As I add features to this portion of the query generator,
though, it will be more useful to test new functionality and also
regression test the framework.

Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
Reviewed-on: http://gerrit.cloudera.org:8080/4081
Reviewed-by: Michael Brown <mi...@cloudera.com>
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Internal Jenkins
---
A tests/comparison/tests/README
A tests/comparison/tests/fake_query.py
A tests/comparison/tests/query_object_testdata.py
A tests/comparison/tests/test_query_objects.py
4 files changed, 335 insertions(+), 0 deletions(-)

Approvals:
  Michael Brown: Looks good to me, but someone else must approve
  Matthew Jacobs: Looks good to me, approved
  Internal Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: 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-4001: qgen: add proof of concept tests for Query() objects

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.

Michael Brown has posted comments on this change.

Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects
......................................................................


Patch Set 3: Code-Review+1

carry +1 since there are no functional changes between patch sets 2 and 3.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
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: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No