Casey Ching has posted comments on this change. Change subject: Query gen: Add INSERT statements ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/2541/1/tests/comparison/db_types.py File tests/comparison/db_types.py: Line 150: return cls.CMP_VALUE + (100 * whole_digits + cls.MAX_DIGITS) / 10000.0 > Can you add a comment about this? Where do the 100 and 10000 magic numbers I changed these classes to use tuples instead. That should be easier to follow. http://gerrit.cloudera.org:8080/#/c/2541/1/tests/comparison/discrepancy_searcher.py File tests/comparison/discrepancy_searcher.py: Line 529: query_generator > let's rename this to statement_generator for consistency Done http://gerrit.cloudera.org:8080/#/c/2541/1/tests/comparison/model_translator.py File tests/comparison/model_translator.py: Line 400: sql = list() > Why not start with an empty string? These could be very big so using a list should be better for performance. I've run into problems using string concat before. Line 406: for idx, col in enumerate(insert.cols): > This looks a bit complicated, how about: Might as well go all the way with this list stuff, I don't think it's too much harder. Line 415: sql.append('), (') > It's kind of hard to follow this logic, I think it would be nicer to rewrit Same as above http://gerrit.cloudera.org:8080/#/c/2541/1/tests/comparison/statement_generator.py File tests/comparison/statement_generator.py: Line 22: class InsertGenerator(object): > Is the idea to run the insert statement both on Impala and Postgres? Will t Ya, eventually there will be deletes too. -- To view, visit http://gerrit.cloudera.org:8080/2541 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6110003789a4b6ae496c02a526c47c3249ef5307 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
