Jackie-Jiang commented on code in PR #9382:
URL: https://github.com/apache/pinot/pull/9382#discussion_r968736701
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -703,7 +705,23 @@ public boolean isBackwardCompatibleWith(Schema oldSchema) {
}
FieldSpec oldSchemaFieldSpec = entry.getValue();
FieldSpec fieldSpec = getFieldSpecFor(oldSchemaColumnName);
- if (!fieldSpec.equals(oldSchemaFieldSpec)) {
+
+ if (EqualityUtils.isSameReference(fieldSpec, oldSchemaFieldSpec)) {
+ continue;
+ }
+
+ if ((fieldSpec == null && oldSchemaFieldSpec != null) || (fieldSpec !=
null && oldSchemaFieldSpec == null)) {
Review Comment:
This check is redundant. We already checked in the previous part and both of
them are not `null`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -1303,14 +1303,20 @@ public void updateSchema(Schema schema, boolean reload)
* Helper method to update the schema, or throw
SchemaBackwardIncompatibleException when the new schema is not
* backward-compatible with the existing schema.
*/
- private void updateSchema(Schema schema, Schema oldSchema)
+ private void updateSchema(Schema schema, Schema oldSchema, boolean force)
throws SchemaBackwardIncompatibleException {
String schemaName = schema.getSchemaName();
schema.updateBooleanFieldsIfNeeded(oldSchema);
if (schema.equals(oldSchema)) {
LOGGER.info("New schema: {} is the same as the existing schema, not
updating it", schemaName);
return;
}
+ if (force) {
Review Comment:
Move this check into the backward compatible check and log a warning when
the schema is not backward compatible and force flag is on.
Don't log warning as info because it can be confusing. We may log a warning
such as `Force updated schema: {} which is backward incompatible with the
existing schema`
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -703,7 +705,23 @@ public boolean isBackwardCompatibleWith(Schema oldSchema) {
}
FieldSpec oldSchemaFieldSpec = entry.getValue();
FieldSpec fieldSpec = getFieldSpecFor(oldSchemaColumnName);
- if (!fieldSpec.equals(oldSchemaFieldSpec)) {
+
+ if (EqualityUtils.isSameReference(fieldSpec, oldSchemaFieldSpec)) {
+ continue;
+ }
+
+ if ((fieldSpec == null && oldSchemaFieldSpec != null) || (fieldSpec !=
null && oldSchemaFieldSpec == null)) {
+ return false;
+ }
+
+ boolean isBackward = EqualityUtils.isEqual(fieldSpec.getName(),
oldSchemaFieldSpec.getName())
Review Comment:
Let's add a method in `FieldSpec`: `boolean
isBackwardCompatibleWith(FieldSpec oldFieldSpec)` so that other sub-classes can
override it if necessary
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java:
##########
@@ -703,7 +705,23 @@ public boolean isBackwardCompatibleWith(Schema oldSchema) {
}
FieldSpec oldSchemaFieldSpec = entry.getValue();
FieldSpec fieldSpec = getFieldSpecFor(oldSchemaColumnName);
- if (!fieldSpec.equals(oldSchemaFieldSpec)) {
+
+ if (EqualityUtils.isSameReference(fieldSpec, oldSchemaFieldSpec)) {
Review Comment:
This check is redundant. They will never share the same reference
--
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]
