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 <[email protected]>
Gerrit-Reviewer: David Knupp <[email protected]>
Gerrit-Reviewer: Matthew Jacobs <[email protected]>
Gerrit-Reviewer: Michael Brown <[email protected]>
Gerrit-Reviewer: Taras Bobrovytsky <[email protected]>
Gerrit-HasComments: Yes

Reply via email to