Jackie-Jiang commented on a change in pull request #5706:
URL: https://github.com/apache/incubator-pinot/pull/5706#discussion_r456140856
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/data/DateTimeGranularitySpec.java
##########
@@ -87,34 +80,33 @@ public TimeUnit getTimeUnit() {
* <ul>
* <li>Convert a granularity to millis.
* This method should not do validation of outputGranularity.
- * The validation should be handled by caller using {@link
#isValidGranularity(String)}</li>
+ * The validation should be handled by caller using {@link
#validateGranularity}</li>
* <ul>
* <li>1) granularityToMillis(1:HOURS) = 3600000 (60*60*1000)</li>
* <li>2) granularityToMillis(1:MILLISECONDS) = 1</li>
* <li>3) granularityToMillis(15:MINUTES) = 900000 (15*60*1000)</li>
* </ul>
* </ul>
- * @return
*/
public Long granularityToMillis() {
return TimeUnit.MILLISECONDS.convert(_size, _timeUnit);
}
/**
* Check correctness of granularity of {@link DateTimeFieldSpec}
- * @param granularity
- * @return
*/
- public static boolean isValidGranularity(String granularity) {
- Preconditions.checkNotNull(granularity);
+ public static void validateGranularity(String granularity) {
+ Preconditions.checkNotNull(granularity, "Granularity string in
dateTimeFieldSpec must not be null");
+
String[] granularityTokens = granularity.split(COLON_SEPARATOR);
- Preconditions.checkArgument(granularityTokens.length ==
MAX_GRANULARITY_TOKENS, GRANULARITY_TOKENS_ERROR_STR);
+ Preconditions.checkArgument(granularityTokens.length ==
MAX_GRANULARITY_TOKENS,
+ "Incorrect granularity:%. Must be of format size:timeunit",
granularity);
Review comment:
```suggestion
"Incorrect granularity: %s. Must be of format 'size:timeunit'",
granularity);
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
##########
@@ -170,9 +170,11 @@ public SuccessResponse addSchema(
@ApiResponses(value = {@ApiResponse(code = 200, message = "Successfully
validated schema"), @ApiResponse(code = 400, message = "Missing or invalid
request body"), @ApiResponse(code = 500, message = "Internal error")})
public String validateSchema(FormDataMultiPart multiPart) {
Schema schema = getSchemaFromMultiPart(multiPart);
- if (!SchemaUtils.validate(schema, LOGGER)) {
- throw new ControllerApplicationException(LOGGER, "Invalid schema. Check
controller logs",
- Response.Status.BAD_REQUEST);
+ try {
+ SchemaUtils.validate(schema);
+ } catch (Exception e) {
+ throw new ControllerApplicationException(LOGGER, String.format("Invalid
schema:%s.", schema.getSchemaName()),
Review comment:
(nit) Space after ':' and no '.' after the schema name for readability.
Same for other exceptions
```suggestion
throw new ControllerApplicationException(LOGGER, "Invalid schema: " +
schema.getSchemaName()),
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java
##########
@@ -417,10 +415,11 @@ private boolean isValid(Schema schema, IndexingConfig
indexingConfig) {
}
}
// 2. We want to get the schema errors, if any, even if isValid is false;
- if (!SchemaUtils.validate(schema, _logger)) {
+ try {
+ SchemaUtils.validate(schema);
+ } catch (Exception e) {
isValid = false;
Review comment:
Log the exception here
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/util/SchemaUtils.java
##########
@@ -37,67 +41,87 @@
public static final String MAP_KEY_COLUMN_SUFFIX = "__KEYS";
public static final String MAP_VALUE_COLUMN_SUFFIX = "__VALUES";
- private static final Logger LOGGER =
LoggerFactory.getLogger(SchemaUtils.class);
-
/**
- * Validates that for a field spec with transform function, the source
column name and destination column name are exclusive
- * i.e. do not allow using source column name for destination column
+ * Validates the following:
+ * 1) Checks valid transform function -
+ * for a field spec with transform function, the source column name and
destination column name are exclusive i.e. do not allow using source column
name for destination column
+ * ensure transform function string can be used to create a {@link
FunctionEvaluator}
+ * 2) Checks for chained transforms/derived transform - not supported yet
+ * TODO: Transform functions have moved to table config. Once we stop
supporting them in schema, remove the validations 1 and 2
+ * 3) Checks valid timeFieldSpec - if incoming and outgoing granularity spec
are different a) the names cannot be same b) cannot use SIMPLE_DATE_FORMAT for
conversion
+ * 4) Checks valid dateTimeFieldSpecs - checks format and granularity string
+ * 5) Schema validations from {@link Schema#validate}
*/
- public static boolean validate(Schema schema) {
- return validate(schema, LOGGER);
+ public static void validate(Schema schema) {
+ schema.validate();
+
+ Set<String> transformedColumns = new HashSet<>();
+ Set<String> argumentColumns = new HashSet<>();
+ for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
+ if (!fieldSpec.isVirtualColumn()) {
+ String column = fieldSpec.getName();
+ String transformFunction = fieldSpec.getTransformFunction();
+ if (transformFunction != null) {
+ try {
+ List<String> arguments =
FunctionEvaluatorFactory.getExpressionEvaluator(fieldSpec).getArguments();
+ if (arguments.contains(column)) {
+ throw new IllegalStateException("The arguments of transform
function '" + transformFunction
+ + "' should not contain the destination column '" + column +
"'");
+ }
Review comment:
For readability. Same for other checks
```suggestion
Preconditions.checkState(arguments.contains(column), "The
arguments of transform function '%s' should not contain the destination column
'%s'", transformFunction, column);
```
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
