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 <[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
