Throw exception when schema name doesn't match table name during table creation

[xiangfu0]

Throw exception when schema doesn't match table name

[Jackie-Jiang]

cc @mcvsubbu @siddharthteotia @yupeng9 @jadami10
We already deprecated the schema name from table config for years (since 2019), and suggest using raw table name as schema name for the following reasons:

• Error prune: sometimes user accidentally copy the table config and point to the wrong schema
• Easier lookup: we can easily look up the schema for a table without going 2 hops, more importantly, we always know the table associated with the schema. Currently we need to scan all the tables in order to find if a schema can be deleted
• Easier management: in the future we can consider managing table config and schema together (we already started this by introducing the TableConfigs concept

Now we want to start the process of enforcing it. Do you have concern about this?
After preventing new table with different schema being created, we will provide an one time migration to rename the schema to match the table config.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.11%. Comparing base (e0a1f62) to head (bd530c5).
Report is 1506 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntroller/helix/core/PinotHelixResourceManager.java	60.00%	2 Missing ⚠️
...he/pinot/segment/local/utils/TableConfigUtils.java	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #11591      +/-   ##
============================================
- Coverage     63.13%   63.11%   -0.03%     
+ Complexity     1118     1117       -1     
============================================
  Files          2342     2342              
  Lines        125883   125882       -1     
  Branches      19357    19357              
============================================
- Hits          79477    79448      -29     
- Misses        40749    40768      +19     
- Partials       5657     5666       +9     

Flag	Coverage Δ
integration	<0.01% <0.00%> (ø)
integration1	<0.01% <0.00%> (ø)
integration2	0.00% <0.00%> (ø)
java-11	63.03% <63.63%> (-0.02%)	⬇️
java-17	62.94% <63.63%> (+0.01%)	⬆️
java-20	62.96% <63.63%> (-0.03%)	⬇️
temurin	63.11% <63.63%> (-0.03%)	⬇️
unittests	63.10% <63.63%> (-0.03%)	⬇️
unittests1	67.25% <66.66%> (-0.04%)	⬇️
unittests2	14.44% <27.27%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mcvsubbu]

It looks like this will cause a problem for us, since some of the users are sending a specific string to indicate that the schema does not exist.

We will dig a little bit more and get back to you.

[mcvsubbu]

So, am I correct that this PR only prevents new tables from getting created without a proper schema name. Existing tables, if they are different schema name, will continue to work as they do now?

I suppose at some point, that will be removed as well, right? (I see code everywhere that corrects the situation for a schema not being present in the same name as the tableName).

[mcvsubbu]

cc: @GSharayu , @sajjad-moradi

[Jackie-Jiang]

@mcvsubbu

So, am I correct that this PR only prevents new tables from getting created without a proper schema name. Existing tables, if they are different schema name, will continue to work as they do now?

Yes, this PR only prevents new tables with different schema name to be created. For the existing tables, we will do a one time migration later to move/copy the schema if they do not match the table name

I suppose at some point, that will be removed as well, right? (I see code everywhere that corrects the situation for a schema not being present in the same name as the tableName).

You mean the schema name field? Once we make all the schema match the table name, we can remove that field

[GSharayu]

Hi @xiangfu0 @Jackie-Jiang ,

As @mcvsubbu mentioned, We are working with partner team to resolve the table creation flow since this will error out for new tables. As I understand from the discussion, this will be followed by a script to apply the changes to the existing tables. We will also need to work with the team for the same. We can get back to you on this PR pretty soon with any updates

[xiangfu0]

Hi @xiangfu0 @Jackie-Jiang ,

As @mcvsubbu mentioned, We are working with partner team to resolve the table creation flow since this will error out for new tables. As I understand from the discussion, this will be followed by a script to apply the changes to the existing tables. We will also need to work with the team for the same. We can get back to you on this PR pretty soon with any updates

Yes, plan to add a periodic task to fix the schema name for existing tables: #11574

This will prevent new table creation.

[yupeng9]

yes, this is a good move.

[mcvsubbu]

I have a more general question on this PR. Should this PR not include places in pinot-spi where we can throw exception if a client tries to create a table config that does not have a schema with the same name? Right now, the TableConfigBuilder class accepts any schema as the schemaName. We should be throwing an exception there, so that these problems are caught earlier?

[xiangfu0]

I have a more general question on this PR. Should this PR not include places in pinot-spi where we can throw exception if a client tries to create a table config that does not have a schema with the same name? Right now, the TableConfigBuilder class accepts any schema as the schemaName. We should be throwing an exception there, so that these problems are caught earlier?

Most of the users are using REST endpoint to POST a json table config to create table.
It's ok to catch earlier in TableConfigBuilder, just the validation cannot be ignored from the server side.

[mcvsubbu]

I have a more general question on this PR. Should this PR not include places in pinot-spi where we can throw exception if a client tries to create a table config that does not have a schema with the same name? Right now, the TableConfigBuilder class accepts any schema as the schemaName. We should be throwing an exception there, so that these problems are caught earlier?

Most of the users are using REST endpoint to POST a json table config to create table. It's ok to catch earlier in TableConfigBuilder, just the validation cannot be ignored from the server side.

In that case, can we please include the change in spi as well? thanks.

We ask our partners to use spi, since that has a better guarantee of keeping backward compat (that is the purpose of spi, right?)

[xiangfu0]

I have a more general question on this PR. Should this PR not include places in pinot-spi where we can throw exception if a client tries to create a table config that does not have a schema with the same name? Right now, the TableConfigBuilder class accepts any schema as the schemaName. We should be throwing an exception there, so that these problems are caught earlier?

Most of the users are using REST endpoint to POST a json table config to create table. It's ok to catch earlier in TableConfigBuilder, just the validation cannot be ignored from the server side.

In that case, can we please include the change in spi as well? thanks.

We ask our partners to use spi, since that has a better guarantee of keeping backward compat (that is the purpose of spi, right?)

Removed the setSchemaName method from TableConfigBuilder

[xiangfu0]

Since we are already at the month end. Let's merge this?
@Jackie-Jiang @mcvsubbu @GSharayu

[Jackie-Jiang]

@mcvsubbu @GSharayu Let us know if we can go ahead and merge this. Once it is merged, the new created table must have schema, and schema name must match raw table name.

[GSharayu]

Hi @xiangfu0 and @Jackie-Jiang , feel free to merge the PR. The changes for new tables have been completed by our partner teams. Thanks a lot!

[xiangfu0]

Hi @xiangfu0 and @Jackie-Jiang , feel free to merge the PR. The changes for new tables have been completed by our partner teams. Thanks a lot!

Thanks!

[estebanz01]

Hola 👋 ! sorry to comment on a merged PR, but I don't see in the release docs for 1.1.0-rc a reference to this change, which is a breaking change for our infrastructure. Is it going to be included in final version of 1.1.0 and where can I find documentation about this?

EDIT: We looked again at the release notes and it's referenced in the bugfixes section.

[vvivekiyer]

@estebanz01 thanks for pointing this out. I've updated the release docs to put this into the backward incompatible section.