impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4001: qgen: add proof of concept tests for Query() objects
Date Mon, 29 Aug 2016 19:03:02 GMT
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


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

PS4, Line 83:   Return a Query object constructed by the keyword args above. select_clause
            :   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?
File tests/comparison/tests/

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:

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <>
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message