Jackie-Jiang commented on code in PR #17645:
URL: https://github.com/apache/pinot/pull/17645#discussion_r2875540997
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -367,7 +368,10 @@ public ConfigSuccessResponse updateConfig(
@ApiParam(value = "Reload the table if the new schema is backward
compatible") @DefaultValue("false")
@QueryParam("reload") boolean reload,
@ApiParam(value = "Force update the table schema")
@DefaultValue("false") @QueryParam("forceTableSchemaUpdate")
- boolean forceTableSchemaUpdate, String tableConfigsStr, @Context
HttpHeaders headers)
+ boolean forceTableSchemaUpdate,
+ @ApiParam(value = "Force update config changes")
+ @QueryParam("force") @DefaultValue("false") boolean force,
Review Comment:
Do we need this extra param? Seems we should just use
`forceTableSchemaUpdate`.
Ideally we should check compatibility before posting the updates
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -840,10 +840,20 @@ public void updateBooleanFieldsIfNeeded(Schema oldSchema)
{
* Backward compatibility requires
* (1) all columns in oldSchema should be retained.
* (2) all column fieldSpecs should be backward compatible with the old ones.
+ * (3) primary key columns should not be changed (used in dimension tables,
upsert, and dedup).
*
* @param oldSchema old schema
*/
public boolean isBackwardCompatibleWith(Schema oldSchema) {
+ // Check primary key columns are not changed
+ List<String> oldPrimaryKeys = oldSchema.getPrimaryKeyColumns();
+ List<String> newPrimaryKeys = getPrimaryKeyColumns();
+ if (oldPrimaryKeys != null && !oldPrimaryKeys.isEmpty()) {
Review Comment:
This basically means we allow setting primary keys (old key doesn't exist).
Is this intentionally? If so, please make it clear in the javadoc
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -840,10 +840,20 @@ public void updateBooleanFieldsIfNeeded(Schema oldSchema)
{
* Backward compatibility requires
* (1) all columns in oldSchema should be retained.
* (2) all column fieldSpecs should be backward compatible with the old ones.
+ * (3) primary key columns should not be changed (used in dimension tables,
upsert, and dedup).
*
* @param oldSchema old schema
*/
public boolean isBackwardCompatibleWith(Schema oldSchema) {
+ // Check primary key columns are not changed
+ List<String> oldPrimaryKeys = oldSchema.getPrimaryKeyColumns();
+ List<String> newPrimaryKeys = getPrimaryKeyColumns();
+ if (oldPrimaryKeys != null && !oldPrimaryKeys.isEmpty()) {
Review Comment:
(nit)
```suggestion
if (CollectionUtils.isNotEmpty(oldPrimaryKeys)) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
