impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Knupp (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4450: qgen: use string concatenation operator for postgres queries
Date Tue, 22 Nov 2016 23:31:27 GMT
David Knupp has posted comments on this change.

Change subject: IMPALA-4450: qgen: use string concatenation operator for postgres queries
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5034/2//COMMIT_MSG
Commit Message:

PS2, Line 22: It's difficult to fully test the effects here. I made sure that we
            : generate syntactically valid queries still on the PostgreSQL side.
The code looks fine to me. I'm curious about this line in the commit message though. Why are
the effects hard to test? If it hadn't been difficult, what other tests would you have wanted
to run -- other than checking for syntactic correctness? Finally, I know you added some unit
tests for the qgen -- would it be appropriate/possible to add a unit test for this function,
e.g., by passing in a mocked func object? If so, is it worth the effort?


-- 
To view, visit http://gerrit.cloudera.org:8080/5034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I149b695889addfd7df4ca5f40dc991456da51687
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message