Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/10656 )
Change subject: Code update for KUDU-2459
......................................................................
Patch Set 4:
Reading through KUDU-2459, I agree that the approach taken here (show the table
name verbatim if it conforms to Impala's requirements, otherwise replace it
with a fixed placeholder) makes sense.
Here are some high level suggestions for improvement:
1. Decompose the table name validation logic into a separate function and add a
unit test that exercises it with both valid and invalid names. I don't think we
have a dedicated "Impala utility" module, but you can start one, perhaps
util/impala-util.{cc,h}?
2. Extend the function to handle _all_ of the Impala identifier requirements.
For example, it'd be nice to account for Impala reserved words as well.
3. Use a placeholder that is more obviously a placeholder.
--
To view, visit http://gerrit.cloudera.org:8080/10656
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 4
Gerrit-Owner: Shriya Gupta <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Marshall <[email protected]>
Gerrit-Comment-Date: Mon, 11 Jun 2018 19:08:26 +0000
Gerrit-HasComments: No
