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