stoty commented on code in PR #1553:
URL: https://github.com/apache/phoenix/pull/1553#discussion_r1084883581
##########
phoenix-core/src/main/antlr3/PhoenixSQL.g:
##########
@@ -704,19 +707,31 @@ column_defs returns [List<ColumnDef> ret]
: v = column_def {$ret.add(v);} (COMMA v = column_def {$ret.add(v);} )*
;
+column_qualifier_counters returns [Map<String, Integer> ret]
Review Comment:
nit:
Call we call this something like "initializiation_list" instead ?
We may want to use this for other cases later.
##########
phoenix-core/src/main/antlr3/PhoenixSQL.g:
##########
@@ -704,19 +707,31 @@ column_defs returns [List<ColumnDef> ret]
: v = column_def {$ret.add(v);} (COMMA v = column_def {$ret.add(v);} )*
;
+column_qualifier_counters returns [Map<String, Integer> ret]
+@init{ret = new HashMap<String,Integer>(); }
+ : k=identifier EQ v=NUMBER {$ret.put(k, Integer.parseInt( v.getText()
));}
+ (COMMA k=identifier EQ v=NUMBER {$ret.put(k, Integer.parseInt(
v.getText() ));} )*
+ ;
+
indexes returns [List<NamedNode> ret]
@init{ret = new ArrayList<NamedNode>(); }
: v = index_name {$ret.add(v);} (COMMA v = index_name {$ret.add(v);} )*
;
column_def returns [ColumnDef ret]
- : c=column_name dt=identifier (LPAREN l=NUMBER (COMMA s=NUMBER)?
RPAREN)? ar=ARRAY? (lsq=LSQUARE (a=NUMBER)? RSQUARE)? (nn=NOT? n=NULL)?
(DEFAULT df=expression)? (pk=PRIMARY KEY (order=ASC|order=DESC)?
rr=ROW_TIMESTAMP?)?
- { $ret = factory.columnDef(c, dt, ar != null || lsq != null, a == null
? null : Integer.parseInt( a.getText() ), nn!=null ? Boolean.FALSE : n!=null ?
Boolean.TRUE : null,
+ : c=column_name dt=identifier (LPAREN l=NUMBER (COMMA s=NUMBER)?
RPAREN)? ar=ARRAY? (lsq=LSQUARE (a=NUMBER)? RSQUARE)? (nn=NOT? n=NULL)?
(DEFAULT df=expression)? ((pk=PRIMARY KEY (order=ASC|order=DESC)?
rr=ROW_TIMESTAMP?)|(COLUMN_QUALIFIER_ID cq=NUMBER))?
Review Comment:
My original intent was to use the encoded binary literal for the CQ here.
Did you have a specific reason to go with the unencoded integer here ?
nit: I'd prefer POSINT instead of number, even if they are aliases, as it is
more expressive.
##########
phoenix-core/src/main/java/org/apache/phoenix/parse/CreateTableStatement.java:
##########
@@ -83,11 +87,13 @@ public CreateTableStatement(CreateTableStatement
createTable, ListMultimap<Strin
this.baseTableName = createTable.baseTableName;
this.whereClause = createTable.whereClause;
this.immutableRows = createTable.immutableRows;
+ this.familyCQCounters = createTable.familyCQCounters;
}
protected CreateTableStatement(TableName tableName,
ListMultimap<String,Pair<String,Object>> props, List<ColumnDef> columns,
PrimaryKeyConstraint pkConstraint,
- List<ParseNode> splitNodes, PTableType tableType, boolean
ifNotExists,
- TableName baseTableName, ParseNode whereClause, int bindCount,
Boolean immutableRows) {
+ List<ParseNode> splitNodes, PTableType
tableType, boolean ifNotExists,
+ TableName baseTableName, ParseNode
whereClause, int bindCount, Boolean immutableRows,
+ Map<String, Integer> familyCounters) {
Review Comment:
I'm not sure String is the good choice here.
The hbase API uses byte[] for families.
It is possible that we already limit the usable families to valid Strings in
Phoenix, but we should double check this.
##########
phoenix-core/src/main/antlr3/PhoenixSQL.g:
##########
@@ -130,6 +130,8 @@ tokens
LIST = 'list';
JARS='jars';
ROW_TIMESTAMP='row_timestamp';
+ COLUMN_QUALIFIER_ID = 'column_qualifier_id';
Review Comment:
Can we just use "column_qualifier" both for the constant name, and the
actual string ?
These are not really IDs.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java:
##########
@@ -2614,7 +2628,47 @@ else if
(!SchemaUtil.isSystemTable(Bytes.toBytes(SchemaUtil.getTableName(schemaN
}
// Use position as column qualifier if APPEND_ONLY_SCHEMA to
prevent gaps in
// the column encoding (PHOENIX-4737).
- Integer encodedCQ = isPkColumn ? null : isAppendOnlySchema ?
Integer.valueOf(ENCODED_CQ_COUNTER_INITIAL_VALUE + position) :
cqCounter.getNextQualifier(cqCounterFamily);
+ Integer encodedCQ = null;
+ if (!isPkColumn) {
Review Comment:
Use curly braces everywhere, it's more readable, and the validators alse
require it.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java:
##########
@@ -2614,7 +2628,47 @@ else if
(!SchemaUtil.isSystemTable(Bytes.toBytes(SchemaUtil.getTableName(schemaN
}
// Use position as column qualifier if APPEND_ONLY_SCHEMA to
prevent gaps in
// the column encoding (PHOENIX-4737).
- Integer encodedCQ = isPkColumn ? null : isAppendOnlySchema ?
Integer.valueOf(ENCODED_CQ_COUNTER_INITIAL_VALUE + position) :
cqCounter.getNextQualifier(cqCounterFamily);
+ Integer encodedCQ = null;
+ if (!isPkColumn) {
+ if (colDef.getColumnQualifier() != null && encodingScheme
!= NON_ENCODED_QUALIFIERS) {
+ if (cqCounter.getNextQualifier(cqCounterFamily) >
ENCODED_CQ_COUNTER_INITIAL_VALUE &&
+ !inputCqCounters.containsKey(cqCounterFamily))
{
+ throw new
SQLExceptionInfo.Builder(SQLExceptionCode.MISSING_CQ)
+ .setSchemaName(schemaName)
+
.setTableName(tableName).build().buildException();
+ }
+
+ if (statement.getFamilyCQCounters() == null ||
+
statement.getFamilyCQCounters().get(cqCounterFamily) == null) {
+ cqCounter.setValue(cqCounterFamily,
colDef.getColumnQualifier());
Review Comment:
Do we already check that the CQs are increasing ?
Looks like this would fail if that's not true.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/tool/SchemaExtractionProcessor.java:
##########
@@ -396,6 +400,35 @@ private TableDescriptor
getTableDescriptor(ConnectionQueryServices cqsi, PTable
}
}
+ private String convertColumnQualifiersToString(PTable table) {
+ StringBuilder cqBuilder = new StringBuilder();
+ if (shouldGenerateWithDefaults)
Review Comment:
I won't add more comments, but make sure you don't use ifs without braces
anywhere.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java:
##########
@@ -2614,7 +2628,47 @@ else if
(!SchemaUtil.isSystemTable(Bytes.toBytes(SchemaUtil.getTableName(schemaN
}
// Use position as column qualifier if APPEND_ONLY_SCHEMA to
prevent gaps in
// the column encoding (PHOENIX-4737).
- Integer encodedCQ = isPkColumn ? null : isAppendOnlySchema ?
Integer.valueOf(ENCODED_CQ_COUNTER_INITIAL_VALUE + position) :
cqCounter.getNextQualifier(cqCounterFamily);
+ Integer encodedCQ = null;
+ if (!isPkColumn) {
+ if (colDef.getColumnQualifier() != null && encodingScheme
!= NON_ENCODED_QUALIFIERS) {
+ if (cqCounter.getNextQualifier(cqCounterFamily) >
ENCODED_CQ_COUNTER_INITIAL_VALUE &&
+ !inputCqCounters.containsKey(cqCounterFamily))
{
+ throw new
SQLExceptionInfo.Builder(SQLExceptionCode.MISSING_CQ)
Review Comment:
So we require a CQ counter if we specify the CQ for column.
Is this really necessary ?
Can't we calculate the new counter value in this case ?
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/tool/SchemaExtractionProcessor.java:
##########
@@ -396,6 +400,35 @@ private TableDescriptor
getTableDescriptor(ConnectionQueryServices cqsi, PTable
}
}
+ private String convertColumnQualifiersToString(PTable table) {
Review Comment:
This should be convertColumnQualifierCountersToString
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
