[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on PHOENIX-3298:
-

Github user lomoree closed the pull request at:

https://github.com/apache/phoenix/pull/214


> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on PHOENIX-3298:
-

Github user lomoree commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/214#discussion_r82305850
  
--- Diff: 
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -1453,7 +1453,7 @@ public void testInvalidPrimaryKeyDecl() throws 
Exception {
 }
 }
 }
-
+
--- End diff --

Of course, I will avoid fixing these in the future. Thanks!


> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on PHOENIX-3298:
-

Github user maryannxue commented on a diff in the pull request:

https://github.com/apache/phoenix/pull/214#discussion_r82285685
  
--- Diff: 
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -1453,7 +1453,7 @@ public void testInvalidPrimaryKeyDecl() throws 
Exception {
 }
 }
 }
-
+
--- End diff --

@lomoree It's nice to avoid these trailing spaces, but most of the time, we 
need to avoid such changes in our patch, especially for 'calcite' branch at the 
moment, for it would make future merge to master more difficult.


> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on PHOENIX-3298:
-

Github user maryannxue commented on the issue:

https://github.com/apache/phoenix/pull/214
  
Sure. I'll do that.


> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on PHOENIX-3298:
-

Github user JamesRTaylor commented on the issue:

https://github.com/apache/phoenix/pull/214
  
+1 to the fix. @chrajeshbabu or @maryannxue - would you mind checking this 
in?


> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[ASF GitHub Bot (JIRA)]

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

ASF GitHub Bot commented on PHOENIX-3298:
-

GitHub user lomoree opened a pull request:

https://github.com/apache/phoenix/pull/214

PHOENIX-3298 Single column primary key may not be null

Note: This patch will not immediately resolve the 
testInvalidPrimaryKeyDecl() test case due to another issue, PHOENIX-3345. 
However, they are independent issues so there's no reason to wait on this.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/bloomberg/phoenix nullcolumns

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/phoenix/pull/214.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #214


commit 669c000c8bea3230f2e895cefebbd00cf6f7d7c6
Author: ERIC LOMORE 
Date:   2016-09-30T06:19:18Z

PHOENIX-3298 Single column primary key may not be null




> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[Eric Lomore (JIRA)]

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

Eric Lomore commented on PHOENIX-3298:
--

Ah! I didn't realize they were non-primitive on the Phoenix side. Will explore 
this, thanks James!

> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[James Taylor (JIRA)]

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

James Taylor commented on PHOENIX-3298:
---

The ColumnDef object in Phoenix declares isNull as Boolean:
{code}
public class ColumnDef {
private final ColumnName columnDefName;
private final PDataType dataType;
private final Boolean isNull;
private final Integer maxLength;
private final Integer scale;
private boolean isPK;
private final SortOrder sortOrder;
private final boolean isArray;
private final Integer arrSize;
private final String expressionStr;
private final boolean isRowTimestamp;

ColumnDef(ColumnName columnDefName, String sqlTypeName, boolean isArray, 
Integer arrSize, Boolean isNull, Integer maxLength,
Integer scale, boolean isPK, SortOrder sortOrder, String 
expressionStr, boolean isRowTimestamp) {
{code}

We've got the following code in MetaDataClient that will flag the problematic 
declaration:
{code}
if (isPK && !addingToPK && pkConstraint.getColumnNames().size() <= 
1) {
if (def.isNull() && def.isNullSet()) {
throw new 
SQLExceptionInfo.Builder(SQLExceptionCode.SINGLE_PK_MAY_NOT_BE_NULL)
.setColumnName(columnName).build().buildException();
}
isNull = false;
}
{code}

Can we declare the isNull as Boolean in parserImpls.ftl and leave it as null if 
not set? Is SqlColumnDefNode derived from ColumnDef or does a translation occur 
to it? Either way, if the ColumnDef isNull Boolean is set appropriately (null, 
true, or false), I don't think there'll be an issue.

> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[Eric Lomore (JIRA)]

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

Eric Lomore commented on PHOENIX-3298:
--

Quick summary of what I was talking about:

During grammar execution, we parse nodes as either Nullable or Not Nullable. If 
no option is stated, then by default, Nullable. No information is preserved on 
whether it was set by default or explicitly.

Once all ColumnDefs are created, and ColumnDefInPkConstraints are created, we 
need to identify the case where there is a single PK column node, and it was 
set to Nullable by default. 

The issue arises in passing this "default"  information down. Hence far, my 2 
ideas were to change the primitive isNull field to a Boolean isNull field, or 
to introduce a new field called implicitlyNull. 
Both of these changes require large changes to the underlying Phoenix code, 
which would cause changes to PhoenixSQL.g which is not what we want because 
this functionality works in Phoenix.
The idea of solving this in the grammar execution doesn't work either since the 
ColumnDefs are constructed long before we have any info on PKConstraints, which 
aren't processed until MetaDataClient.java.

See the attached patch that would convert "Nullable" to "Not Nullable" for ANY 
single column primary key (not just those implicitly set).

The relevant grammar code:

{code:java|title=parserImpls.ftl}
SqlColumnDefNode ColumnDef() :
{
SqlIdentifier columnName;
SqlDataTypeNode dataType;
boolean isNull = true;
boolean isPk = false;
SortOrder sortOrder = SortOrder.getDefault();
boolean isRowTimestamp = false;
SqlParserPos pos;
}
{
columnName = DualIdentifier()
dataType = PhoenixDataType()
[
 
{isNull = false;}
|

{isNull = true;}
]
[
 
{isPk = true;}
]
[

{sortOrder = SortOrder.ASC;}
|

{sortOrder = SortOrder.DESC;}
]
[

{isRowTimestamp = true;}
]
{
pos = columnName.getParserPosition().plus(getPos());
return new SqlColumnDefNode(pos, columnName, dataType, isNull, isPk, 
sortOrder, null, isRowTimestamp);
}
}
{code}

> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
> Attachments: PHOENIX-3298-WIP
>
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[James Taylor (JIRA)]

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

James Taylor commented on PHOENIX-3298:
---

Yes, we'd still want to throw an exception in that case. In general, we 
shouldn't have to change any unit tests (and if we do, we need to be very 
careful to note any incompatibilities we're introducing). We can introduce a 
change in MetaDataClient if that's easier (as [~maryannxue] suggested). I think 
the change won't be a pure grammar change. You can likely make the change in 
the grammar rule execution, though, when the ColumnDef object is created.

> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[Eric Lomore (JIRA)]

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

Eric Lomore commented on PHOENIX-3298:
--

Before I finish up on this, I wanted to find out the desired functionality for 
a case like the one below. Do we want to still throw an error for explicit 
(incorrect) null declarations or is it okay to also convert this to not null, 
maybe with some type of warning posted?
If we do want to still throw an error for this statement, I believe a small 
grammar change would be necessary to add a field indicating whether "isNull" is 
explicitly or implicitly set in SqlOptionNode.

  {code} @Test
public void testInvalidPrimaryKeyDecl() throws Exception {
String[] queries = {
"CREATE TABLE t (k varchar null primary key)",
"CREATE TABLE t (k varchar null, constraint pk primary key 
(k))",
};
Connection conn = DriverManager.getConnection(getUrl());
for (String query : queries) {
try {
conn.createStatement().execute(query);
fail("Compilation should have failed since this is an invalid 
PRIMARY KEY declaration: " + query);
} catch (SQLException e) {
System.out.println(e.getMessage());
assertEquals(query, 
SQLExceptionCode.SINGLE_PK_MAY_NOT_BE_NULL.getErrorCode(), e.getErrorCode());
}
}
}{code}

> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>Assignee: Eric Lomore
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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

[jira] [Commented] (PHOENIX-3298) Create Table: Single column primary key may not be null

[James Taylor (JIRA)]

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

James Taylor commented on PHOENIX-3298:
---

Phoenix assumes that a single column PK is not nullable because it doesn't 
allow a non nullable single column PK. Since we're parsing the create table 
statement, seems like we can handle this ourselves. We could deprecate this 
behavior potentially too and change the tests.

> Create Table: Single column primary key may not be null
> ---
>
> Key: PHOENIX-3298
> URL: https://issues.apache.org/jira/browse/PHOENIX-3298
> Project: Phoenix
>  Issue Type: Sub-task
>Reporter: Eric Lomore
>
> Create table statements with a single column currently must have "NOT NULL" 
> identifier to pass tests.
> Running this code results in failure
> {code}CREATE TABLE t (k VARCHAR PRIMARY KEY DESC){code}
> While this allows tests to pass
> {code}CREATE TABLE t (k VARCHAR NOT NULL PRIMARY KEY DESC){code}
> Must either enforce the not null condition and update test cases, or apply a 
> fix.



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