[GitHub] [samza] weiqingy commented on a change in pull request #1153: SAMZA-2320: Samza-sql: Refactor validation to cover more cases and make it more extensible.

[GitBox <gi...@apache.org>]

weiqingy commented on a change in pull request #1153: SAMZA-2320: Samza-sql: Refactor validation to cover more cases and make it more extensible.
URL: https://github.com/apache/samza/pull/1153#discussion_r324300490
 
 

 ##########
 File path: samza-sql/src/main/java/org/apache/samza/sql/planner/SamzaSqlValidator.java
 ##########
 @@ -86,33 +88,46 @@ public void validate(List<String> sqlStmts) throws SamzaSqlValidatorException {
     }
   }
 
-  protected void validate(RelRoot relRoot, SamzaSqlQueryParser.QueryInfo qinfo, SamzaSqlApplicationConfig sqlConfig)
+  private void validate(RelRoot relRoot, SamzaSqlQueryParser.QueryInfo qinfo, SamzaSqlApplicationConfig sqlConfig)
       throws SamzaSqlValidatorException {
-    // Validate select fields (including Udf return types) with output schema
-    validateOutput(relRoot, sqlConfig.getRelSchemaProviders().get(qinfo.getSink()));
+    if (!skipOutputValidation(relRoot, qinfo, sqlConfig)) {
+      // Validate select fields (including Udf return types) with output schema
+      validateOutput(relRoot, sqlConfig.getRelSchemaProviders().get(qinfo.getSink()));
+    }
 
     // TODO:
     //  1. SAMZA-2314: Validate Udf arguments.
     //  2. SAMZA-2315: Validate operators. These are the operators that are supported by Calcite but not by Samza Sql.
     //     Eg: LogicalAggregate with sum function is not supported by Samza Sql.
   }
 
-  protected void validateOutput(RelRoot relRoot, RelSchemaProvider relSchemaProvider) throws SamzaSqlValidatorException {
-    RelRecordType outputRecord = (RelRecordType) QueryPlanner.getSourceRelSchema(relSchemaProvider,
+  private void validateOutput(RelRoot relRoot, RelSchemaProvider outputRelSchemaProvider)
+      throws SamzaSqlValidatorException {
+    LogicalProject project = (LogicalProject) relRoot.rel;
+    RelRecordType projetRecord = (RelRecordType) project.getRowType();
+    RelRecordType outputRecord = (RelRecordType) QueryPlanner.getSourceRelSchema(outputRelSchemaProvider,
         new RelSchemaConverter());
     // Get Samza Sql schema along with Calcite schema. The reason is that the Calcite schema does not have a way
     // to represent optional fields while Samza Sql schema can represent optional fields. This is the only reason that
     // we use SqlSchema in validating output.
-    SqlSchema outputSqlSchema = QueryPlanner.getSourceSqlSchema(relSchemaProvider);
+    SqlSchema outputSqlSchema = QueryPlanner.getSourceSqlSchema(outputRelSchemaProvider);
 
-    LogicalProject project = (LogicalProject) relRoot.rel;
-    RelRecordType projetRecord = (RelRecordType) project.getRowType();
+    validateOutputRecords(outputRecord, outputSqlSchema, projetRecord, outputRelSchemaProvider);
+    LOG.info("Samza Sql Validation finished successfully.");
+  }
+
+  protected boolean skipOutputValidation(RelRoot relRoot, SamzaSqlQueryParser.QueryInfo qinfo,
+      SamzaSqlApplicationConfig sqlConfig) {
+    return false;
+  }
 
-    validateOutputRecords(outputRecord, outputSqlSchema, projetRecord);
+  protected boolean isOptional(RelSchemaProvider outputRelSchemaProvider, String outputFieldName,
 
 Review comment:
   Could you add some comments saying why isOptional and skipOutputValidation alway return false?

----------------------------------------------------------------
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


With regards,
Apache Git Services