PHOENIX-6457 - Optionally store schema version string in SYSTEM.CATALOG by gjacoby126 · Pull Request #1216 · apache/phoenix

gjacoby126 · 2021-04-30T17:38:45Z

No description provided.

stoty · 2021-04-30T21:40:24Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	5m 37s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s	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.
		_ 4.x Compile Tests _
+1 💚	mvninstall	15m 7s	4.x passed
+1 💚	compile	1m 6s	4.x passed
+1 💚	checkstyle	3m 12s	4.x passed
+1 💚	javadoc	0m 49s	4.x passed
+0 🆗	spotbugs	3m 17s	phoenix-core in 4.x has 945 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	6m 53s	the patch passed
+1 💚	compile	1m 6s	the patch passed
+1 💚	cc	1m 6s	the patch passed
+1 💚	javac	1m 6s	the patch passed
-1 ❌	checkstyle	3m 14s	phoenix-core: The patch generated 76 new + 10260 unchanged - 25 fixed = 10336 total (was 10285)
+1 💚	prototool	0m 1s	There were no new prototool issues.
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 48s	the patch passed
-1 ❌	spotbugs	3m 30s	phoenix-core generated 1 new + 944 unchanged - 1 fixed = 945 total (was 945)
		_ Other Tests _
+1 💚	unit	195m 15s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 11s	The patch does not generate ASF License warnings.
		241m 2s

Reason	Tests
FindBugs	module:phoenix-core
	org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.SCHEMA_VERSION_BYTES is a mutable array At MetaDataEndpointImpl.java: At MetaDataEndpointImpl.java:[line 344]

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1216
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool
uname	Linux e090cf77c106 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	4.x / `282427d`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstyle	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
spotbugs	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/1/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/1/testReport/
Max. process+thread count	5076 (vs. ulimit of 30000)
modules	C: phoenix-core U: phoenix-core
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/1/console
versions	git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

swaroopak · 2021-05-03T17:00:23Z

-import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.VIEW_INDEX_ID_DATA_TYPE;
-import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.VIEW_STATEMENT;
-import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.VIEW_TYPE;
+import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.*;


nit: unnecessary change.

Oops, thought I'd fixed all those before pushing. Fixed and repushed.

swaroopak · 2021-05-03T17:01:25Z

 }

+ if (schemaVersion == null) {
+ tableUpsert.setNull(34, Types.VARCHAR);


Or should have a default starting schema version?

I'd considered it but decided against the idea because I didn't want to proscribe a particular versioning scheme...the idea is that a particular organization can choose their own version stamps (and that they don't have to if they don't want to, hence default null)

swaroopak · 2021-05-03T17:05:38Z

+ String alterViewSql = "ALTER VIEW " + viewFullName + " SET SCHEMA_VERSION='" + newVersion + "'";
+ conn.createStatement().execute(alterViewSql);
+ PTable view2 = PhoenixRuntime.getTableNoCache(conn, viewFullName);
+ assertEquals(newVersion, view2.getSchemaVersion());


should we assert on changes on datatable schema version as well?

There's not meant to be any cascade behavior here...what changes should I assert on? Just that that the base table didn't change? I could do that.

swaroopak · 2021-05-03T17:10:50Z

+ throws Exception {
+ final String dataTableFullName = SchemaUtil.getTableName(schemaName, tableName);
+ String ddl =
+ "CREATE TABLE " + dataTableFullName + " (\n" + "ID1 VARCHAR(15) NOT NULL,\n"


If there are any changes on the table schema like new column addition or drop of an existing column, how would the schema version be updated? I am thinking phoenix should increment the version or we need to enhance the alter DDL to include schema version also in the statement. If the former, schema version format should be fixed just like sequence number.

It appears there's a rule in Phoenix grammar which prevents setting table level properties (like schema version) while adding a column, which means that when adding a column you'd need a separate ALTER TABLE SET SCHEMA_VERSION statement. Not ideal, but I was trying to avoid large changes in the plumbing.

If I support changing Storage format or Column encoding, we will need to do alter table set schema again right? I think it will be good to invest in plumbing this change.

@gokceni - wouldn't those be table properties as well? Should be able to do in one statement.

Good to have a follow-up Jira in that case. I feel ALTER ADD/DROP should implicitly increment the schema_version. If every org/customer decides the schema_version format by themselves, this will be difficult. WDYT?

Agree with @swaroopak that we should do it.

I disagree that schema version should auto-increment. First, because an auto-incrementing int sequence already exists (see PTable.getSequenceNumber, which gets incremented by DDL statements), and second, because that's not really what this feature is meant to do.
The purpose of SCHEMA_VERSION is that it's a string which is relevant to a particular client application or applications and its release numbering.
Say application Foo has quarterly releases "Q1-2021", "Q1-2021.1", "Q2-2021", and so on. It creates tables A, B, and C as part of Q1-2021, makes a table property change to table B during Q1-2021.1, and then adds 2 more tables D and E and 1 column to an existing table C in Q2-2021.
At this point the schema versions are:
A: Q1-2021
B: Q1-2021.1
C: Q2-2021 (assuming there was a separate statement to update schema version after adding the column)
D: Q2-2021
E: Q2-2021
From this, the application Foo can check for each table that its schema version is the version that it expects (or not) and take action accordingly. An application running Q1-2021 can still interpret queries / messages against Table A, but might want to reject ones from tables B through E, because they're from newer schemas that it has no knowledge of.

As @swaroopak points out, it's a pain to have to do the extra ALTER TABLE SET SCHEMA_VERSION statement after doing an add/drop column, so it would be nice if we could update this property as part of doing the add or drop column. I'll see if I can figure out the cause of why the existing restriction is there, and if it can be easily set aside.

Thanks for the explanation. This makes sense

swaroopak · 2021-05-03T17:12:56Z

 }

+ @Test
+ public void testCreateIndexSchemaVersion() throws Exception {


How will the select statements behave? They need to be schema version aware at least implicitly.

Not sure I understand this point...

Let's say I drop the column and schema version changes from v1 to v1.1. Currently, the phoenix will use v1.1 when processing a select * . can we add a test for that?
And is there a way I can query with old schema_version?

Phoenix won't really "use" a schema version at all during a select. To be precise, when it grabs the PTable to be able to interpret the query results, the schema version will be in the PTable, but it's not really used by the query logic.
The point of schema version is to allow:
Applications to check to see if a table, view or index is compatible with a particular version of their schema
In the future, when we have change streaming from Phoenix, messages will be annotated with the schema version so consumers of the message will be able to check to see if they can interpret the message (i.e, it's compatible with a particular version of a consuming application) or not.

gokceni · 2021-05-03T20:49:08Z

+ throws Exception {
+ final String dataTableFullName = SchemaUtil.getTableName(schemaName, tableName);
+ String ddl =
+ "CREATE TABLE " + dataTableFullName + " (\n" + "ID1 VARCHAR(15) NOT NULL,\n"


If I support changing Storage format or Column encoding, we will need to do alter table set schema again right? I think it will be good to invest in plumbing this change.

gokceni · 2021-05-03T20:50:08Z

 public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_15_0 = MIN_TABLE_TIMESTAMP + 29;
 public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_16_0 = MIN_TABLE_TIMESTAMP + 33;
- public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_17_0 = MIN_TABLE_TIMESTAMP + 34;
+ public static final long MIN_SYSTEM_TABLE_TIMESTAMP_4_17_0 = MIN_TABLE_TIMESTAMP + 35;


How do we come up with 35?

Each syscat column needs an individual timestamp, and 33 was 4.16 + 1 for Physical table name, +1 for schema version

stoty · 2021-05-04T00:18:48Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 11s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s	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.
		_ 4.x Compile Tests _
+1 💚	mvninstall	14m 51s	4.x passed
+1 💚	compile	1m 5s	4.x passed
+1 💚	checkstyle	3m 7s	4.x passed
+1 💚	javadoc	0m 49s	4.x passed
+0 🆗	spotbugs	3m 17s	phoenix-core in 4.x has 945 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	6m 46s	the patch passed
+1 💚	compile	1m 6s	the patch passed
+1 💚	cc	1m 6s	the patch passed
+1 💚	javac	1m 6s	the patch passed
-1 ❌	checkstyle	3m 15s	phoenix-core: The patch generated 75 new + 10260 unchanged - 25 fixed = 10335 total (was 10285)
+1 💚	prototool	0m 1s	There were no new prototool issues.
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 46s	the patch passed
-1 ❌	spotbugs	3m 33s	phoenix-core generated 1 new + 944 unchanged - 1 fixed = 945 total (was 945)
		_ Other Tests _
+1 💚	unit	196m 41s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 9s	The patch does not generate ASF License warnings.
		237m 33s

Reason	Tests
FindBugs	module:phoenix-core
	org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.SCHEMA_VERSION_BYTES is a mutable array At MetaDataEndpointImpl.java: At MetaDataEndpointImpl.java:[line 344]

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1216
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile cc prototool
uname	Linux f793742f68bc 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	4.x / `21bfbad`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstyle	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
spotbugs	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/2/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/2/testReport/
Max. process+thread count	5093 (vs. ulimit of 30000)
modules	C: phoenix-core U: phoenix-core
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1216/2/console
versions	git=2.7.4 maven=3.3.9 spotbugs=4.1.3 prototool=1.10.0-dev
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

swaroopak

Thank you for addressing the comments and answering questions. +1, LGTM

…OG (#1216) * PHOENIX-6457 - Optionally store schema version string in SYSTEM.CATALOG

PHOENIX-6457 - Optionally store schema version string in SYSTEM.CATALOG
e4ac9e0

gjacoby126 requested a review from swaroopak April 30, 2021 17:38

swaroopak reviewed May 3, 2021
View reviewed changes

PHOENIX-6457 - Optionally store schema version string in SYSTEM.CATALOG
1e56d2d

gokceni reviewed May 3, 2021
View reviewed changes

swaroopak approved these changes May 5, 2021
View reviewed changes

gokceni approved these changes May 5, 2021
View reviewed changes

gjacoby126 merged commit ddc3996 into apache:4.xMay 6, 2021

gjacoby126 mentioned this pull request May 10, 2021
PHOENIX-6457 - Optionally store schema version string in SYSTEM.CATALOG #1225
Merged

swaroopak pushed a commit that referenced this pull request May 21, 2021
PHOENIX-6457- Optionally store schema version string in SYSTEM.CATAL…
5fabbe8
…OG (#1216) * PHOENIX-6457 - Optionally store schema version string in SYSTEM.CATALOG

Uh oh!

Conversation

gjacoby126 commented Apr 30, 2021

Uh oh!

stoty commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gjacoby126May 4, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty commented May 4, 2021

Uh oh!

swaroopak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gjacoby126May 4, 2021•
edited
Loading