[
https://issues.apache.org/jira/browse/PHOENIX-6720?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17680138#comment-17680138
]
ASF GitHub Bot commented on PHOENIX-6720:
-----------------------------------------
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
> "create table" can't recreate column encoded tables that had columns dropped
> ----------------------------------------------------------------------------
>
> Key: PHOENIX-6720
> URL: https://issues.apache.org/jira/browse/PHOENIX-6720
> Project: Phoenix
> Issue Type: Bug
> Components: core
> Affects Versions: 5.2.0, 5.1.2
> Reporter: Istvan Toth
> Assignee: Aron Attila Meszaros
> Priority: Blocker
>
> For column encoded tables,create table generate column qualifier order.
> When moving data between instances, and the original table had some columns
> removed, or swapped around, then the column qualifers in the table are not
> guarenteed to start from the expected value and increase by 1 for each
> column, based on the ordering of columns.
> This means that when we load a data table via HBase (i.e from a snaphsot),
> and then execute the DDL created by the show create table, or by other means,
> the column_qualifiers on the new table are going to point to the wrong Hbase
> cell.
> We need to accept and use COLUMN_QUALIFIER properties for columns, and
> include them in in the show create table output for column encoded tables.
> We also need to accept and generate QUALIFIER_COUNTER for the tables.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
