PHOENIX-6720 "create table" can't recreate column encoded tables that had columns dropped #1553

Aarchy · 2023-01-23T15:38:16Z

No description provided.

phoenix-core/src/main/antlr3/PhoenixSQL.g

stoty · 2023-01-24T07:19:33Z

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]


nit:
Call we call this something like "initializiation_list" instead ?
We may want to use this for other cases later.

stoty · 2023-01-24T07:30:06Z

phoenix-core/src/main/antlr3/PhoenixSQL.g

 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))?


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.

What re the advantages of using encoded binary literals?
I believe these checks are more readable this way:
cq.getValue() < QueryConstants.ENCODED_CQ_COUNTER_INITIAL_VALUE

We can use that internal representation independent of the user-facing format.

I've just verified, that the "COLUMN_QUALIFIER" syscat column always stores the qualifier, whether we use encoded qualifiers, or unencoded ones.

On the other hand, there is no way to specify the unencoded COLUMN_QUALIFER directly, so that does not rule out using the integer values here.

We should make sure to use a name that makes it clear this is only for encoded qualifiers, like
ENCODED_QUALIFER.

stoty · 2023-01-24T07:41:35Z

phoenix-core/src/main/java/org/apache/phoenix/parse/CreateTableStatement.java

-            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) {


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.

ColumnDef class stores family names as NamedNode, which is a String and a boolean flag for case sensitivity. So using String here was a problem, but not because hbase, rather because the case sensitive cases.

Family names for the counters are now stored as NamedNode, also few new tests have been added.

Thank you for pointing this out.

stoty · 2023-01-24T07:58:31Z

phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

+                    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)


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 ?

This is not quite the case here. This checks validates if every column has its qualifier. eg.
COL1 INTEGER, COL2 INTEGER COLUMN_QUALIFIER 13
In this case I don't think we can really figure out what the qualifier of COL1 should be. (it could be 11, 12)

stoty · 2023-01-24T08:06:00Z

phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java

+
+                        if (statement.getFamilyCQCounters() == null ||
+                                statement.getFamilyCQCounters().get(cqCounterFamily) == null) {
+                            cqCounter.setValue(cqCounterFamily, colDef.getColumnQualifier());


Do we already check that the CQs are increasing ?
Looks like this would fail if that's not true.

Fixed, thank you.

stoty · 2023-01-24T08:08:58Z

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) {


Use curly braces everywhere, it's more readable, and the validators alse require it.

stoty · 2023-01-24T08:21:12Z

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)


I won't add more comments, but make sure you don't use ifs without braces anywhere.

stoty · 2023-01-24T08:22:23Z

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) {


This should be convertColumnQualifierCountersToString

stoty · 2023-01-25T19:40:51Z

We already have the DEFAULT_COLUMN_FAMILY property, where we specify column families in a table property.

According to the grammar page:

DEFAULT_COLUMN_FAMILY string option determines the column family used used when none is specified. The value is case sensitive. If this option is not present, a column family name of '0' is used.

So we should stick to the same semantics, and not do the "normalize to uppercase when not double quoted" stuff here.

We could argue that your current solution is more correct, but we should have a single way of handling families (at least in table properties)

stoty · 2023-01-25T19:42:50Z

i.e.
COLUMN_QUALIFIER_COUNTER ('A'=14, 'B'=13, 'a'=5)

Of course it's also worth checking the the docs I quoted above is correct.

stoty

Looks good, I have one nit with a parameter/variable naming.

stoty · 2023-01-30T06:49:14Z

phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java

+    public ColumnDef columnDef(ColumnName columnDefName, String sqlTypeName,
+                               boolean isArray, Integer arrSize, Boolean isNull,
+                               Integer maxLength, Integer scale, boolean isPK,
+                               SortOrder sortOrder, String expressionStr, Integer cq, boolean isRowTimestamp) {


I'd prefer 'encodedQualifier' or instead of 'cq' as a the variable name.

stoty

+1 LGTM

Aron Meszaros and others added 11 commits January 23, 2023 16:31

modify show create table to export qualifiers

7f59111

parse CQs in create table stmt

aa0a9ac

input validation and unittests

4efa758

clean up

71a04db

exception handling, integration tests, IMMUTABLE_STORAGE_SCHEME

362426a

tests

0b6b727

missing CQ, write counters optionally, test fixes

e87901c

fix SchemaExtractionProcessorIT, change grammar)

cc28b34

fix AlterTableIT

f8fb35d

fix tests WIP

570f855

fix TransformMonitorExtendedIT

315d065

stoty reviewed Jan 24, 2023

View reviewed changes

PR fixes

999e183

read column family as string literal

dff842f

stoty requested changes Jan 30, 2023

View reviewed changes

change cq to eq

4a6a2b4

stoty approved these changes Jan 30, 2023

View reviewed changes

stoty closed this Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-6720 "create table" can't recreate column encoded tables that had columns dropped #1553

PHOENIX-6720 "create table" can't recreate column encoded tables that had columns dropped #1553

Aarchy commented Jan 23, 2023

stoty Jan 24, 2023

stoty Jan 24, 2023

Aarchy Jan 24, 2023

stoty Jan 24, 2023

stoty Jan 24, 2023

Aarchy Jan 25, 2023

stoty Jan 24, 2023

Aarchy Jan 24, 2023 •

edited

Loading

stoty Jan 24, 2023

stoty Jan 24, 2023

Aarchy Jan 25, 2023

stoty Jan 24, 2023

stoty Jan 24, 2023

stoty Jan 24, 2023

stoty commented Jan 25, 2023

stoty commented Jan 25, 2023

stoty left a comment

stoty Jan 30, 2023

stoty left a comment

PHOENIX-6720 "create table" can't recreate column encoded tables that had columns dropped #1553

PHOENIX-6720 "create table" can't recreate column encoded tables that had columns dropped #1553

Conversation

Aarchy commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aarchy Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty commented Jan 25, 2023

stoty commented Jan 25, 2023

stoty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty left a comment

Choose a reason for hiding this comment

Aarchy Jan 24, 2023 •

edited

Loading