You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2017/11/15 09:05:47 UTC
[Impala-ASF-CR] IMPALA-5362 : Preserve case-sensitivity in field titles
ydjainopensource@gmail.com has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8544
Change subject: IMPALA-5362 : Preserve case-sensitivity in field titles
......................................................................
IMPALA-5362 : Preserve case-sensitivity in field titles
To preserve case sensitivity in column labels , this patch modifies the getColumnLabel()
method of the SelectListitem class to return original string. However since internally
all identifiers are represented in lowercase a few minor changes have been made to
SelectStmt.java.lowercase "null" was causing union test to fail hence replaced it with "NULL"
Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
---
M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
3 files changed, 16 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/8544/1
--
To view, visit http://gerrit.cloudera.org:8080/8544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
Gerrit-Change-Number: 8544
Gerrit-PatchSet: 1
Gerrit-Owner: ydjainopensource@gmail.com
[Impala-ASF-CR] IMPALA-5362 : Preserve case-sensitivity in field titles
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8544 )
Change subject: IMPALA-5362 : Preserve case-sensitivity in field titles
......................................................................
Patch Set 1:
> I think this is a pretty dangerous change with the potential to
> break compatibility in non-trivial ways. I'd prefer to abandon for
> now unless there is a strong reason to continue down this path.
>
> Changing the column labels will affect columns stored in the HMS
> via CTAS, and it will change column aliases created for inline
> views and such. Not sure how preserving the case might break
> things, but this change will definitely need paranoid testing. My
> preference would be to defer this change.
I'm on board with that.
--
To view, visit http://gerrit.cloudera.org:8080/8544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
Gerrit-Change-Number: 8544
Gerrit-PatchSet: 1
Gerrit-Owner: ydjainopensource@gmail.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Comment-Date: Sun, 26 Nov 2017 20:32:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5362 : Preserve case-sensitivity in field titles
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8544 )
Change subject: IMPALA-5362 : Preserve case-sensitivity in field titles
......................................................................
Patch Set 1:
(10 comments)
Testing (in a dry run; will not talk back to gerrit) here: https://jenkins.impala.io/job/gerrit-verify-dryrun/1474/
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@9
PS1, Line 9:
nit: no space before comma
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@9
PS1, Line 9: To preserve case sensitivity in column labels , this patch modifies the getColumnLabel()
Please wrap commit messages at 72 characters. You can do this in emacs (if you use emacs for your commit message writing) by hitting ctrl-q.
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@10
PS1, Line 10: return original
"return the original"
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@10
PS1, Line 10: However since
nit: "However, since"
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@11
PS1, Line 11: lowercase a
nit: "lowercase, a"
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@12
PS1, Line 12: SelectStmt.java.lowercase
This is not a packagae name. Reword?
http://gerrit.cloudera.org:8080/#/c/8544/1//COMMIT_MSG@12
PS1, Line 12: causing union test to fail hence replaced
nit: "causing the union test to fail, hence I replace"
http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
File fe/src/main/java/org/apache/impala/analysis/SelectListItem.java:
http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@97
PS1, Line 97: lower case
remove
http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@98
PS1, Line 98: lower case
remove
http://gerrit.cloudera.org:8080/#/c/8544/1/fe/src/main/java/org/apache/impala/analysis/SelectListItem.java@99
PS1, Line 99: lower case
remove
--
To view, visit http://gerrit.cloudera.org:8080/8544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
Gerrit-Change-Number: 8544
Gerrit-PatchSet: 1
Gerrit-Owner: ydjainopensource@gmail.com
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Comment-Date: Wed, 15 Nov 2017 18:22:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5362 : Preserve case-sensitivity in field titles
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has abandoned this change. ( http://gerrit.cloudera.org:8080/8544 )
Change subject: IMPALA-5362 : Preserve case-sensitivity in field titles
......................................................................
Abandoned
See my comment on the danger of this change. I don't think the risk is worth the reward at this point. Due to compatibility issues I don't think this is a task suitable for a newbie either.
Really sorry about the miscommunication.
--
To view, visit http://gerrit.cloudera.org:8080/8544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
Gerrit-Change-Number: 8544
Gerrit-PatchSet: 1
Gerrit-Owner: ydjainopensource@gmail.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
[Impala-ASF-CR] IMPALA-5362 : Preserve case-sensitivity in field titles
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8544 )
Change subject: IMPALA-5362 : Preserve case-sensitivity in field titles
......................................................................
Patch Set 1:
I think this is a pretty dangerous change with the potential to break compatibility in non-trivial ways. I'd prefer to abandon for now unless there is a strong reason to continue down this path.
Changing the column labels will affect columns stored in the HMS via CTAS, and it will change column aliases created for inline views and such. Not sure how preserving the case might break things, but this change will definitely need paranoid testing. My preference would be to defer this change.
--
To view, visit http://gerrit.cloudera.org:8080/8544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia574f0b7d5bdbebace270ce4079632bf29b3f00e
Gerrit-Change-Number: 8544
Gerrit-PatchSet: 1
Gerrit-Owner: ydjainopensource@gmail.com
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Comment-Date: Tue, 21 Nov 2017 23:14:00 +0000
Gerrit-HasComments: No