impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database
Date Thu, 22 Sep 2016 22:50:31 GMT
Taras Bobrovytsky has posted comments on this change.

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

Patch Set 7:

File tests/comparison/

Line 100:     if default is None:
I think it's a little confusing to check if default is None here.

How about:

result = self._hadoop_configs.get(key, default)
if result is None:
  raise ...
return result

Line 185:     other_conf_dir = os.environ["HIVE_CONF_DIR"]
I'm not sure that modifying the minicluster here is the right thing to do. It represents the
minicluster environment in the Impala repository. How are you running, the Hive tests? Do
you use this environment or you run against a cluster?
File tests/comparison/

Line 92:     self.db_name = None
How about adding a variable here called, for example, db_engine that can be either set to
Hive or Impala? Then we won't have to worry about passing is_hive to populate_db every time.

Line 153:         cursor.compute_stats()
We don't need to compute stats if we are using hive?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 7
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

View raw message