has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database

Patch Set 3:

Commit Message:

PS2, Line 7: IMPALA-3980: qgen: re-enable Hive as a target database
> I prefer commit messages that, after the bug, speak of the code section cha

PS2, Line 19: * Hive integration tested locally by invoking the data generator 
via the command:
            : ./ \
            : --db-name=functional \
            : --use-hive \
            : --min-row-count=50 \
> While it makes the commit message have more lines, it is more readable to w
File tests/comparison/

PS2, Line 150:   group.add_argument('--hive-host', default=DEFAULT_HIVE_HOST,
             :       help="The name of the host running the HS2")
             :   group.add_argument("--hive-port", default=DEFAULT_HIVE_PORT, 
             :       help="The port of HiveServer2")
> Why not derive these defaults from symbols imported from (The c
File tests/comparison/

PS2, Line 166: class MiniCluster(Cluster):
> This changes the MiniCluster constructor interface for all callers. It also

PS2, Line 178:   def _init_local_hadoop_conf_dir(self):
> Thanks for doing the cleanups like this one.
No problem! It makes the code a lot easier to run locally.
File tests/comparison/

PS2, Line 101:   def populate_db(self, table_count, postgresql_conn=None, 
> We don't use variable names of the format "isHive" typically, so can you ch

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Reviewer: David Knupp <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

Reply via email to