You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/09/03 02:49:13 UTC
[GitHub] [incubator-pinot] npawar opened a new pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
npawar opened a new pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966
## Description
Add validation to check that timeColumnName is in sync across the table config and schema.
**Cases**
Adding/updating tableConfig - Find associated schema (rawTableName or from validationConfig.getSchemaName). Check that timeColumnName is present in schema.
Adding/updating schema - Find associated table configs (schemaName_OFFLINE and schemaName_REALTIME) Check timeColumnName in all associated tableConfigs also exists in schema.
Corner case: If tableConfig has been created with a schemaName != raw tableName, then we will not be able to validate during schema add/upload.
## Release Notes
After this change,
1. uploading a tableConfig with timeColumnName not present in schema will be disallowed (provided schema already uploaded)
2. uploading schema with timeColumnName not present in the schema will be disallowed (provided schemaName = tableName)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#issuecomment-687284187
> We do allow to have tables without a schema. I have not had the time to go through this fully, but do you account for the fact that a (offline) table may not have a schema at all?
Yes, I have accounted for offline table not having schema at all. I've put in the description all the cases it covers, and the case we miss out
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#issuecomment-686624956
We do allow to have tables without a schema. I have not had the time to go through this fully, but do you account for the fact that a (offline) table may not have a schema at all?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486488972
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -171,7 +173,8 @@ public SuccessResponse addSchema(
public String validateSchema(FormDataMultiPart multiPart) {
Schema schema = getSchemaFromMultiPart(multiPart);
try {
- SchemaUtils.validate(schema);
+ List<TableConfig> tableConfigs = getTableConfigsForSchema(schema.getSchemaName());
Review comment:
i thought about that. I was just trying to avoid passing pinotHelixResourceManager to the utils methods. I can change it to that if you think that's better. I was going back and forth about it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486588035
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/util/SchemaUtilsTest.java
##########
@@ -35,6 +43,115 @@
*/
public class SchemaUtilsTest {
+ private static final String TABLE_NAME = "testTable";
+ private static final String TIME_COLUMN = "timeColumn";
+
+ @Test
+ public void testCompatibilityWithTableConfig() {
+ // empty list
+ List<TableConfig> tableConfigs = new ArrayList<>();
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME).build();
+ SchemaUtils.validate(schema, tableConfigs);
Review comment:
I meant catching exception and explicitly 'fail'ing. It's fine though - just a nit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r485792320
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -171,7 +173,8 @@ public SuccessResponse addSchema(
public String validateSchema(FormDataMultiPart multiPart) {
Schema schema = getSchemaFromMultiPart(multiPart);
try {
- SchemaUtils.validate(schema);
+ List<TableConfig> tableConfigs = getTableConfigsForSchema(schema.getSchemaName());
Review comment:
I wonder if this (along with the getTableConfigsForSchema function) can be moved inside validate ? So that the caller doesn't have to do this every single time (and can be done implicitly)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r485796322
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
##########
@@ -74,19 +80,37 @@ public static void validateTableName(TableConfig tableConfig) {
}
}
- private static void validateValidationConfig(TableConfig tableConfig) {
+ /**
+ * Validates the following in the validationConfig of the table
+ * 1. For REALTIME table
+ * - checks for non-null timeColumnName
+ * - checks for valid field spec for timeColumnName in schema
+ *
+ * 2. For OFFLINE table
+ * - checks for valid field spec for timeColumnName in schema, if timeColumnName and schema re non-null
+ *
+ * 3. Checks peerDownloadSchema
+ */
+ private static void validateValidationConfig(TableConfig tableConfig, @Nullable Schema schema) {
SegmentsValidationAndRetentionConfig validationConfig = tableConfig.getValidationConfig();
- if (validationConfig != null) {
- if (tableConfig.getTableType() == TableType.REALTIME && validationConfig.getTimeColumnName() == null) {
- throw new IllegalStateException("Must provide time column in real-time table config");
- }
- String peerSegmentDownloadScheme = validationConfig.getPeerSegmentDownloadScheme();
- if (peerSegmentDownloadScheme != null) {
- if (!CommonConstants.HTTP_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme)
- && !CommonConstants.HTTPS_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme)) {
- throw new IllegalStateException("Invalid value '" + peerSegmentDownloadScheme
- + "' for peerSegmentDownloadScheme. Must be one of http nor https");
- }
+ String timeColumnName = validationConfig.getTimeColumnName();
+ if (tableConfig.getTableType() == TableType.REALTIME) {
+ // For REALTIME table, must have a non-null timeColumnName
+ Preconditions.checkState(timeColumnName != null, "'timeColumnName' cannot be null in REALTIME table config");
+ }
+ // timeColumnName can be null in OFFLINE table
+ if (timeColumnName != null && schema != null) {
+ Preconditions.checkState(schema.getSpecForTimeColumn(timeColumnName) != null,
+ "Cannot find valid fieldSpec for timeColumn: %s from the table config, in the schema: %s", timeColumnName,
+ schema.getSchemaName());
+ }
+
+ String peerSegmentDownloadScheme = validationConfig.getPeerSegmentDownloadScheme();
+ if (peerSegmentDownloadScheme != null) {
+ if (!CommonConstants.HTTP_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme) && !CommonConstants.HTTPS_PROTOCOL
Review comment:
Curious - how is this related to time column field validation ?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableIndexingConfigs.java
##########
@@ -58,7 +59,8 @@ public SuccessResponse updateIndexingConfig(
TableConfig tableConfig;
try {
tableConfig = JsonUtils.stringToObject(tableConfigString, TableConfig.class);
- TableConfigUtils.validate(tableConfig);
+ Schema schema = pinotHelixResourceManager.getSchemaForTableConfig(tableConfig);
Review comment:
Same here
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/util/SchemaUtilsTest.java
##########
@@ -35,6 +43,115 @@
*/
public class SchemaUtilsTest {
+ private static final String TABLE_NAME = "testTable";
+ private static final String TIME_COLUMN = "timeColumn";
+
+ @Test
+ public void testCompatibilityWithTableConfig() {
+ // empty list
+ List<TableConfig> tableConfigs = new ArrayList<>();
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME).build();
+ SchemaUtils.validate(schema, tableConfigs);
Review comment:
nit: wrap with try-catch for future proofing ?
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/util/SchemaUtilsTest.java
##########
@@ -35,6 +43,115 @@
*/
public class SchemaUtilsTest {
+ private static final String TABLE_NAME = "testTable";
+ private static final String TIME_COLUMN = "timeColumn";
+
+ @Test
+ public void testCompatibilityWithTableConfig() {
+ // empty list
+ List<TableConfig> tableConfigs = new ArrayList<>();
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME).build();
+ SchemaUtils.validate(schema, tableConfigs);
+
+ // offline table
+ // null timeColumnName
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME).build();
+ SchemaUtils.validate(schema, Lists.newArrayList(tableConfig));
Review comment:
Same here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486562121
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -318,4 +324,19 @@ private void deleteSchemaInternal(String schemaName) {
Response.Status.INTERNAL_SERVER_ERROR);
}
}
+
+ private List<TableConfig> getTableConfigsForSchema(@Nullable String schemaName) {
Review comment:
`schemaName` should never be null
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -318,4 +324,19 @@ private void deleteSchemaInternal(String schemaName) {
Response.Status.INTERNAL_SERVER_ERROR);
}
}
+
+ private List<TableConfig> getTableConfigsForSchema(@Nullable String schemaName) {
Review comment:
Wondering if we can move this method into `PinotHelixResourceManager`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar merged pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar merged pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486484365
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/util/SchemaUtilsTest.java
##########
@@ -35,6 +43,115 @@
*/
public class SchemaUtilsTest {
+ private static final String TABLE_NAME = "testTable";
+ private static final String TIME_COLUMN = "timeColumn";
+
+ @Test
+ public void testCompatibilityWithTableConfig() {
+ // empty list
+ List<TableConfig> tableConfigs = new ArrayList<>();
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME).build();
+ SchemaUtils.validate(schema, tableConfigs);
Review comment:
do you mean a case where the schema becomes invalid, but we pass the test anyway because of not having a try-catch?
Just to be sure, I tried this out. The test fails if there's any exception in the should-pass cases.
Lmk if you were referring to something else.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486480697
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/TableConfigUtils.java
##########
@@ -74,19 +80,37 @@ public static void validateTableName(TableConfig tableConfig) {
}
}
- private static void validateValidationConfig(TableConfig tableConfig) {
+ /**
+ * Validates the following in the validationConfig of the table
+ * 1. For REALTIME table
+ * - checks for non-null timeColumnName
+ * - checks for valid field spec for timeColumnName in schema
+ *
+ * 2. For OFFLINE table
+ * - checks for valid field spec for timeColumnName in schema, if timeColumnName and schema re non-null
+ *
+ * 3. Checks peerDownloadSchema
+ */
+ private static void validateValidationConfig(TableConfig tableConfig, @Nullable Schema schema) {
SegmentsValidationAndRetentionConfig validationConfig = tableConfig.getValidationConfig();
- if (validationConfig != null) {
- if (tableConfig.getTableType() == TableType.REALTIME && validationConfig.getTimeColumnName() == null) {
- throw new IllegalStateException("Must provide time column in real-time table config");
- }
- String peerSegmentDownloadScheme = validationConfig.getPeerSegmentDownloadScheme();
- if (peerSegmentDownloadScheme != null) {
- if (!CommonConstants.HTTP_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme)
- && !CommonConstants.HTTPS_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme)) {
- throw new IllegalStateException("Invalid value '" + peerSegmentDownloadScheme
- + "' for peerSegmentDownloadScheme. Must be one of http nor https");
- }
+ String timeColumnName = validationConfig.getTimeColumnName();
+ if (tableConfig.getTableType() == TableType.REALTIME) {
+ // For REALTIME table, must have a non-null timeColumnName
+ Preconditions.checkState(timeColumnName != null, "'timeColumnName' cannot be null in REALTIME table config");
+ }
+ // timeColumnName can be null in OFFLINE table
+ if (timeColumnName != null && schema != null) {
+ Preconditions.checkState(schema.getSpecForTimeColumn(timeColumnName) != null,
+ "Cannot find valid fieldSpec for timeColumn: %s from the table config, in the schema: %s", timeColumnName,
+ schema.getSchemaName());
+ }
+
+ String peerSegmentDownloadScheme = validationConfig.getPeerSegmentDownloadScheme();
+ if (peerSegmentDownloadScheme != null) {
+ if (!CommonConstants.HTTP_PROTOCOL.equalsIgnoreCase(peerSegmentDownloadScheme) && !CommonConstants.HTTPS_PROTOCOL
Review comment:
This part was already present in the validateValidationConfig method (line 84 in the before diff)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486497471
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -171,7 +173,8 @@ public SuccessResponse addSchema(
public String validateSchema(FormDataMultiPart multiPart) {
Schema schema = getSchemaFromMultiPart(multiPart);
try {
- SchemaUtils.validate(schema);
+ List<TableConfig> tableConfigs = getTableConfigsForSchema(schema.getSchemaName());
Review comment:
Actually, as I tried to do this, I realized that PinotHelixResourceManager is in pinot-controller, and the utils (which are in pinot-core) do not have access to it. We'd have to pass propertyStore, or change the design of SchemaUtils so we can inject and keep state.
Now I think it's better that the caller fetches what it needs. Practically, PinotTableRestletResource and PinotSchemaRestletResource should be the only places adding/modifying table/schema.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486601450
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -318,4 +324,19 @@ private void deleteSchemaInternal(String schemaName) {
Response.Status.INTERNAL_SERVER_ERROR);
}
}
+
+ private List<TableConfig> getTableConfigsForSchema(@Nullable String schemaName) {
Review comment:
yes, done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] icefury71 commented on a change in pull request #5966: Validate timeColumnName when adding/updating schema/tableConfig
Posted by GitBox <gi...@apache.org>.
icefury71 commented on a change in pull request #5966:
URL: https://github.com/apache/incubator-pinot/pull/5966#discussion_r486512420
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -171,7 +173,8 @@ public SuccessResponse addSchema(
public String validateSchema(FormDataMultiPart multiPart) {
Schema schema = getSchemaFromMultiPart(multiPart);
try {
- SchemaUtils.validate(schema);
+ List<TableConfig> tableConfigs = getTableConfigsForSchema(schema.getSchemaName());
Review comment:
Understood. Thanks for looking into this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org