[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-22 Thread James Taylor (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15514561#comment-15514561
 ] 

James Taylor commented on PHOENIX-3263:
---

+1. Thanks, [~lomoree]. I committed this on your behalf.

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>Assignee: Eric Lomore
> Attachments: PHOENIX-3263.patch
>
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-22 Thread Hadoop QA (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15514535#comment-15514535
 ] 

Hadoop QA commented on PHOENIX-3263:


{color:red}-1 overall{color}.  Here are the results of testing the latest 
attachment 
  http://issues.apache.org/jira/secure/attachment/12829925/PHOENIX-3263.patch
  against master branch at commit b1682ddd541031437d2731c570a54fc6494c9801.
  ATTACHMENT ID: 12829925

{color:green}+1 @author{color}.  The patch does not contain any @author 
tags.

{color:red}-1 tests included{color}.  The patch doesn't appear to include 
any new or modified tests.
Please justify why no new tests are needed for this 
patch.
Also please list what manual steps were performed to 
verify this patch.

{color:red}-1 patch{color}.  The patch command could not apply the patch.

Console output: 
https://builds.apache.org/job/PreCommit-PHOENIX-Build/593//console

This message is automatically generated.

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>Assignee: Eric Lomore
> Attachments: PHOENIX-3263.patch
>
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-22 Thread James Taylor (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15513709#comment-15513709
 ] 

James Taylor commented on PHOENIX-3263:
---

[~lomoree] - would you mind attaching a patch file so we can get this fix 
committed? To generate a patch file:
- commit your fix to your local repo with a commit message of the form 
"PHOENIX-### ", so for this one it'd be "PHOENIX-3263 Allow comma 
before CONSTRAINT to be optional"
- create a patch file by doing {{git format-patch --stdout HEAD^ > 
PHOENIX-.patch}}, so for this one it'd be {{git format-patch --stdout HEAD^ 
> PHOENIX-3263.patch}}
- attach the patch file to this JIRA using the More->Attach Files

For more on this process, see https://phoenix.apache.org/contributing.html

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-13 Thread Maryann Xue (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15488424#comment-15488424
 ] 

Maryann Xue commented on PHOENIX-3263:
--

Thanks for you response, [~lomoree]! But you won't be able to commit it :) You 
can either create a fork of the Apache Phoenix project on github and work on 
the "calcite" branch of your fork, or submit a patch to this JIRA issue. Then 
after it passes the review, I can commit it for you.

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-13 Thread Eric Lomore (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15488400#comment-15488400
 ] 

Eric Lomore commented on PHOENIX-3263:
--

Thanks! This makes a lot more sense now, still getting used to the structure of 
everything.
As expected it was a simple fix, going to do some final testing and I will 
commit it tomorrow.

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-13 Thread Maryann Xue (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15487974#comment-15487974
 ] 

Maryann Xue commented on PHOENIX-3263:
--

Hey, [~lomoree], in Calcite-Phoenix we are no longer using the original parser 
code in Phoenix (I believe one of my slides mentioned it). Instead, you should 
look at files under "phoenix-core/src/main/codegen/", and for this case, 
specifically at "phoenix-core/src/main/codegen/includes/parserImpls.ftl".

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-13 Thread James Taylor (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15487914#comment-15487914
 ] 

James Taylor commented on PHOENIX-3263:
---

[~lomoree] - I don't completely understand this because Calcite doesn't provide 
any parsing for DDL - Phoenix is doing this parsing. Take a look back at the 
commit that [~maryannxue] did to support CREATE TABLE, in particular this 
file[1]. We wouldn't want to have to parse using the existing Phoenix antlr 
parser - in the end, we'll get rid of our parser completely.

[1] 
https://git-wip-us.apache.org/repos/asf?p=phoenix.git;a=blobdiff;f=phoenix-core/src/main/codegen/includes/parserImpls.ftl;h=e84e404cbaf0922426e894a4e6ef4220fef0afb7;hp=f7c203b2ec58ff9f39a5a25b5f78eba0ef6c20ef;hb=af237d5608c23ad418d24a24db3eb4aead5fc8f0;hpb=f344e3355ea1257144edde8306a1fe76675f5ab6

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (PHOENIX-3263) Allow comma before CONSTRAINT to be optional

2016-09-13 Thread Eric Lomore (JIRA) 

[ 
https://issues.apache.org/jira/browse/PHOENIX-3263?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15487831#comment-15487831
 ] 

Eric Lomore commented on PHOENIX-3263:
--

At the moment, Phoenix supports both comma and no comma in front of the 
constraint, and Phoenix-Calcite supports only no comma in front of the 
constraint.

The reason phoenix is able to do this is because it immediately parses the SQL 
query into a CompilableStatement and then executes it.
{code:title=PhoenixStatement.java|borderStyle=solid}
CompilableStatement stmt = parseStatement(sql);
executeQuery(stmt);
{code}

In Phoenix-Calcite the raw SQL query is handed down to Calcite logic. Hence, we 
could raise this issue in a Calcite jira, since the standard is to support both 
and Calcite does not.
Alternatively, we can add a parsing layer on top of Phoenix-calcite to remove 
the comma and then pass it to the Calcite execution runtime.

{code:title=AvaticaStatement.java|borderStyle=solid}
Meta.ExecuteResult x = connection.prepareAndExecuteInternal(this, sql, 
maxRowCount1);
{code}

To further illustrate that I don't believe this is a phoenix issue, the 
following test case passes on the calcite branch.

{code}
@Test
public void testParseCreateTableCommaBeforePrimaryKeyConstraint() throws 
Exception {
for (String leadingComma : new String[]{",", ""}) {
String s = "create table core.entity_history_archive (id CHAR(15), 
name VARCHAR(150)${o} constraint pk primary key (id))".replace("${o}", 
leadingComma);

CreateTableStatement stmt = (CreateTableStatement)new 
SQLParser((s)).parseStatement();
assertEquals(2, stmt.getColumnDefs().size());
assertNotNull(stmt.getPrimaryKeyConstraint());
}
}
{code}

> Allow comma before CONSTRAINT to be optional
> 
>
> Key: PHOENIX-3263
> URL: https://issues.apache.org/jira/browse/PHOENIX-3263
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: James Taylor
>
> In Phoenix, the comma before the CONSTRAINT is optional (which matches 
> Oracle). Can this be supported in Calcite Phoenix?
> For example, this is ok in Phoenix:
> {code}
> CREATE TABLE T (
> K VARCHAR
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> as is this:
> {code}
> CREATE TABLE T (
> K VARCHAR,
> CONSTRAINT PK PRIMARY KEY (K));
> {code}
> If this is not feasible, we could require the comma and change the tests. 
> This is leading to a lot of failures.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)