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