You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Shriya Gupta (Code Review)" <ge...@cloudera.org> on 2018/06/08 18:59:08 UTC

[kudu-CR] Code update for KUDU-2459

Shriya Gupta has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10656


Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 13 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/1
-- 
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: newchange
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 1
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>

[kudu-CR] Code update for KUDU-2459

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Mon, 11 Jun 2018 19:08:26 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#14).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
metrics display a CREATE TABLE statement that can be run to make Impala
cognizant of that table. However, in generation of this statement, the
tablename tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to derive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results. We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore with this update,
invalid tablenames have been identified and updated by a placeholder
tablename for end user to replace with a valid tablename. The rules to
check Impala conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 22 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/14
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 14
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#5).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 18 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/5
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 5
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................


Patch Set 12:

(7 comments)

I filed KUDU-2523 for the test failure, which looks unrelated to your patch.

http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@12
PS12, Line 12: tablename
Across this commit message (and the code), you're writing both "table name" and "tablename". Can you pick one term and use it consistently?


http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17
PS12, Line 17: drive
derive


http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19
PS12, Line 19: We
Nit: space before "We"


http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@21
PS12, Line 21: invalid tablenames have been identified and updated by a placeholder 
             : tablename for end user to replace with a valid tablename. The rules to 
Nit: you have trailing whitespace on these two lines.


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269
PS5, Line 269:       if (orig_name.length() <= 128) {
> Combining these if-conditions exceeds the total code line length requiremen
I don't think a combined if statement with line breaks is so bad:

  if (!orig_name.empty() &&
      orig_name.length() <= 128 &&
      ascii_isalpha(orig_name[0]) &&
      find_if(orig_name.begin(), orig_name.end(), [](char c) {
                return !(isalnum(c) || (c == '_'));
              }) == orig_name.end()) {
    impala_name = orig_name;
}


http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@268
PS12, Line 268:       //Ensuring non-zero-length tablename
Style nit: format single-line comments as "// foo bar" not "//foo bar".


http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270
PS12, Line 270:         //Ensuring that the tablename length does not exceed 128 characters
I think these comments would be more readable if grouped into a single block at the beginning. Something like:

  // Not all Kudu table names are also valid Impala identifiers. We need to replace such names
  // with a placeholder because ...
  //
  // Valid Impala identifiers conform to the following rules:
  // 1. Must not be empty.
  // 2. Length must not exceed 128 characters.
  // 3. First character must be alphabetic.
  // 4. Every character must be alphanumeric or an underscore.



-- 
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: 12
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 01 Aug 2018 17:13:45 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................


Patch Set 13:

(7 comments)

> (7 comments)
 > 
 > I filed KUDU-2523 for the test failure, which looks unrelated to
 > your patch.

Yep, thank you, I wasn't sure why a commit message update on a successful build would warrant/fail a debug build test. I have made the other updates too.

http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@12
PS12, Line 12: ablename 
> Across this commit message (and the code), you're writing both "table name"
Done


http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@17
PS12, Line 17: deriv
> derive
Done


http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@19
PS12, Line 19:  W
> Nit: space before "We"
Done


http://gerrit.cloudera.org:8080/#/c/10656/12//COMMIT_MSG@21
PS12, Line 21: invalid tablenames have been identified and updated by a placeholder
             : tablename for end user to replace with a valid tablename. The rules to
> Nit: you have trailing whitespace on these two lines.
Done


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269
PS5, Line 269:     // 2. Length must not exceed 128 characters.
> I don't think a combined if statement with line breaks is so bad:
Done


http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@268
PS12, Line 268:     // 1. Must not be empty.
> Style nit: format single-line comments as "// foo bar" not "//foo bar".
Done


http://gerrit.cloudera.org:8080/#/c/10656/12/src/kudu/master/master_path_handlers.cc@270
PS12, Line 270:     // 3. First character must be alphabetic.
> I think these comments would be more readable if grouped into a single bloc
Done



-- 
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: 13
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 01 Aug 2018 18:16:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#15).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
metrics display a CREATE TABLE statement that can be run to make Impala
cognizant of that table. However, in generation of this statement, the
tablename tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to derive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results. We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore with this update,
invalid tablenames have been identified and updated by a placeholder
tablename for end user to replace with a valid tablename. The rules to
check Impala conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 25 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/15
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 15
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#4).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 18 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/4
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 4
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
metrics display a CREATE TABLE statement that can be run to make Impala
cognizant of that table. However, in generation of this statement, the
tablename tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to derive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results. We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore with this update,
invalid tablenames have been identified and updated by a placeholder
tablename for end user to replace with a valid tablename. The rules to
check Impala conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Reviewed-on: http://gerrit.cloudera.org:8080/10656
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 28 insertions(+), 1 deletion(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
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: merged
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 17
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#3).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 16 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/3
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 3
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................


Patch Set 16: Code-Review+2


-- 
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: 16
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 01 Aug 2018 20:40:11 +0000
Gerrit-HasComments: No

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#7).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 18 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/7
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 7
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#13).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
metrics display a CREATE TABLE statement that can be run to make Impala
cognizant of that table. However, in generation of this statement, the
tablename tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to derive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results. We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore with this update,
invalid tablenames have been identified and updated by a placeholder
tablename for end user to replace with a valid tablename. The rules to
check Impala conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 22 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/13
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 13
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#9).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
 metrics display a CREATE TABLE statement that can be run to make Impala
 cognizant of that table. However, in generation of this statement, the 
table name tries to match the original Kudu tablename which may not 
always be acceptable as a tablename for Impala. For example, Kudu 
accepts dot in tablename, Impala does not. The CREATE TABLE statement 
thus throws an invalid tablename error in Impala.

We considered trying to drive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive 
results.We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore, in this update, 
only invalidablenames have been updated by a placeholder tablename for 
end user to replace with a valid tablename. The rules to check Impala
conformity are below -- 

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character. 
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the 
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 25 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/9
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 9
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#6).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 18 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/6
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 6
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#10).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
 metrics display a CREATE TABLE statement that can be run to make Impala
 cognizant of that table. However, in generation of this statement, the
table name tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to drive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results.We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore, in this update,
only invalidablenames have been updated by a placeholder tablename for
end user to replace with a valid tablename. The rules to check Impala
conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 25 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/10
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 10
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#12).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
metrics display a CREATE TABLE statement that can be run to make Impala
cognizant of that table. However, in generation of this statement, the
table name tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to drive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results.We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore with this update,
invalid tablenames have been identified and updated by a placeholder 
tablename for end user to replace with a valid tablename. The rules to 
check Impala conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 24 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/12
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 12
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................


Patch Set 16:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@265
PS14, Line 265: 
              :     // Not all Kudu tablenames are also valid Impala identifiers. We need to
              :     // replace such names with a placeholder when they are used as Impala
> These lines look like they're too long: please wrap at 80 characters.
Done


http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@273
PS14, Line 273:     // 4. Every character must be alphanumeric or an underscore.
> Style nit: when you want to emphasize that a comment applies to a particula
That's a good pointer. Done.



-- 
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: 16
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 01 Aug 2018 20:08:11 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................


Patch Set 11:

(8 comments)

Updates made per the comments.

http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG@7
PS5, Line 7: KUDU-2459: add placeholder names to some CREATE TABLE statements
> We typically format commit messages as:
Done


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@266
PS5, Line 266:     string orig_name = l.data().name();
             :     if (!orig_name.empty()) {
> Don't need "std::" prefixes; there's a "using std::string" at the top of th
Done


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@268
PS5, Line 268:       //Ensuring non-zero-length tablename
> Do !orig_name.empty() instead; it's more idiomatic.
Done


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269
PS5, Line 269:       if (orig_name.length() <= 128) {
> Please combine these conditions rather than nesting more and more ifs toget
Combining these if-conditions exceeds the total code line length requirement. I broke it into separate conditions for more readability purposes. I have added code comments. I can still concatenate if you feel that's better, but with the line breaks, the logic appears more complex to follow.


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@270
PS5, Line 270:         //Ensuring that the tablename length does not exceed 128 characters
> Add a space between the operator (>=) and each of the operands (orig_name[0
Done


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@272
PS5, Line 272:           //Ensuring first character of tablename is alphabetic
             :           if (find_if(orig_name.begin(), orig_name.end(),[](char c) {
> Reformat this so it's more obvious that "return ..." is the body of a lambd
Done


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@275
PS5, Line 275:             //For every character in the name, it checks whether the character is alphanumeric
> Won't this cause HandleTablePage to return early and not finish outputting 
I have removed the return statement.


http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache
File www/table.mustache:

http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache@156
PS5, Line 156: name
> Does this need to be replaced with a placeholder too?
No, this is the actual kudu tablename, it needs to be carried here as is.



-- 
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: 11
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 01 Aug 2018 02:41:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#11).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
 metrics display a CREATE TABLE statement that can be run to make Impala
 cognizant of that table. However, in generation of this statement, the
table name tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to drive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results.We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore, in this update,
only invalidablenames have been updated by a placeholder tablename for
end user to replace with a valid tablename. The rules to check Impala
conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 24 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/11
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 11
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#8).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 27 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/8
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 8
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#2).

Change subject: Code update for KUDU-2459
......................................................................

Code update for KUDU-2459

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 16 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/2
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 2
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Code update for KUDU-2459

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: Code update for KUDU-2459
......................................................................


Patch Set 5:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/10656/5//COMMIT_MSG@7
PS5, Line 7: Code update for KUDU-2459
We typically format commit messages as:

  <bug number if applicable>: <short description of fix>

  <longer description of fix>

So in your case it could be:

  KUDU-2459: add placeholder names to some CREATE TABLE statements

  <explain the problem. explain the solutions considered, and why this one was adopted>

Remember to wrap each line to 80 characters (https://kudu.apache.org/docs/contributing.html#_line_length).


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@266
PS5, Line 266:     std::string impala_name = "<placeholder>";
             :     std::string orig_name = l.data().name();
Don't need "std::" prefixes; there's a "using std::string" at the top of the file.


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@268
PS5, Line 268:     if (orig_name.length() >= 1) {
Do !orig_name.empty() instead; it's more idiomatic.


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@269
PS5, Line 269:       if (orig_name.length() <= 128) {
Please combine these conditions rather than nesting more and more ifs together.

Please also document the constraints in a more human-readable way via code comment.


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@270
PS5, Line 270:         if ((orig_name[0]>='a' && orig_name[0] <= 'z')
Add a space between the operator (>=) and each of the operands (orig_name[0] and 'a').

Actually, just use ascii_isalpha() from gutil/strings/ascii_ctype.h. Below too use ascii_isalnum() instead of isalnum() from <ctype.h>.


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@272
PS5, Line 272:           if (find_if(orig_name.begin(), orig_name.end(),[](char c) {
             :             return !(isalnum(c) || (c == '_')); }) == orig_name.end())
Reformat this so it's more obvious that "return ..." is the body of a lambda. Something like this:

  if (find_if(orig_name.begin(), orig_name.end(), [](char c) {
                return !(isalnum(c) || (c == '_'));
              }) == orig_name.end()) {
    ...
  }


http://gerrit.cloudera.org:8080/#/c/10656/5/src/kudu/master/master_path_handlers.cc@275
PS5, Line 275:             return;
Won't this cause HandleTablePage to return early and not finish outputting below?

This is why I think it'd still be good to decompose this into a helper method, because then it's easy to test, and you could have caught this mistake.


http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache
File www/table.mustache:

http://gerrit.cloudera.org:8080/#/c/10656/5/www/table.mustache@156
PS5, Line 156: name
Does this need to be replaced with a placeholder too?



-- 
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: 5
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 31 Jul 2018 18:19:38 +0000
Gerrit-HasComments: Yes

[kudu-CR] Code update for KUDU-2459

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Shriya Gupta has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: Code update for KUDU-2459
......................................................................


Patch Set 5:

Hey Adar, 

Thank you for the code review. From our discussion offline, I have the addressed the improvement suggestions as below. Please let me know if this is acceptable.

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}?
-With HMS Integration landing soon, this work is not needed for current functionality improvement

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.
-This functionality is not needed as the tablename is placed in double-quotes, an exception that Impala allows for identifier names.

3. Use a placeholder that is more obviously a placeholder.
-I have replaced the placeholder with '<placeholder>' thereby forcing the user to be mindful about updating it.

Thanks,
Shriya


-- 
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: 5
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Tue, 31 Jul 2018 16:47:39 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10656 )

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................


Patch Set 14:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@265
PS14, Line 265:     // Not all Kudu tablenames are also valid Impala identifiers. We need to replace such names
              :     // with a placeholder when they are used as Impala identifiers, for example as Impala tablename
              :     // in the Kudu Web UI. Valid Impala identifiers conform to the following rules:
These lines look like they're too long: please wrap at 80 characters.


http://gerrit.cloudera.org:8080/#/c/10656/14/src/kudu/master/master_path_handlers.cc@273
PS14, Line 273:     string impala_name = "<placeholder>";
Style nit: when you want to emphasize that a comment applies to a particular block of code, format it like this:

  <some other code>

  <comment>
  <applicable block of code>

  <other code>

What's important here? That the comment and the block of code aren't separated by an empty line, and that the two are separated from the code before and after with empty lines. This helps emphasize that the comment and the code should be considered together.



-- 
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: 14
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 01 Aug 2018 18:58:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2459: add placeholder names to some CREATE TABLE statements

Posted by "Shriya Gupta (Code Review)" <ge...@cloudera.org>.

Hello Thomas Marshall, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/10656

to look at the new patch set (#16).

Change subject: KUDU-2459: add placeholder names to some CREATE TABLE statements
......................................................................

KUDU-2459: add placeholder names to some CREATE TABLE statements

Under the Tables section of Kudu Web UI, for a selected table, the table
metrics display a CREATE TABLE statement that can be run to make Impala
cognizant of that table. However, in generation of this statement, the
tablename tries to match the original Kudu tablename which may not
always be acceptable as a tablename for Impala. For example, Kudu
accepts dot in tablename, Impala does not. The CREATE TABLE statement
thus throws an invalid tablename error in Impala.

We considered trying to derive an Impala-conforming name from the Kudu-
supplied tablename but that could result in surprising/unintuitive
results. We also want to leverage the current functionality of auto-
generated statement where the name is valid. Therefore with this update,
invalid tablenames have been identified and updated by a placeholder
tablename for end user to replace with a valid tablename. The rules to
check Impala conformity are below --

- The minimum length of an identifier is 1 character.

- The maximum length of an identifier is currently 128 characters.

- An identifier must start with an alphabetic character.
The remainder can contain any combination of alphanumeric characters and
underscores. Quoting the identifier with backticks has no effect on the
allowed characters in the name.

- An identifier can contain only ASCII characters.

Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
---
M src/kudu/master/master_path_handlers.cc
M www/table.mustache
2 files changed, 28 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/10656/16
-- 
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: newpatchset
Gerrit-Change-Id: If9e5242318589452a0507d3973b9be3e6a186a69
Gerrit-Change-Number: 10656
Gerrit-PatchSet: 16
Gerrit-Owner: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Shriya Gupta <sh...@gmail.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>