Repository navigation
SAMZA-2316: Validate that all non-default value fields in output schema are set in the projected fields. - #1149
Conversation
…n the projected fields.
| SqlSchemaBuilder schemaBuilder = SqlSchemaBuilder.builder(); | ||
| for (Schema.Field field : fields) { | ||
| SqlFieldSchema fieldSchema = convertField(field.schema()); | ||
| // Consider any field with default value as nullable. Is it the right assumption ? |
There was a problem hiding this comment.
This is slightly tricky. Nullable fields in avro is of type union with one of the value being null.
The fields with default value means they are optional during serializing and deserializing.
The behavior on the consumption side.
- All avro fields should have values including nullable fields as well as optional fields(fields with default values)
Behavior on the producer side.
- fields with default values are optional
- Nullable fields may still need values if they are not optional
So nullability and optionality are slightly different. I am not sure it is a good idea to conflate them.
There was a problem hiding this comment.
Thanks for clarifying the behavior. Updated the logic by separating out nullable fields and fields with default values. Using default values for validation but not nullable fields.
|
Nit: copy the words in the description to the title? |
…n the projected fields.
atoomula
changed the title
…n the projected fields.
…n the projected fields.
…n the projected fields.
|
|
||
| private SqlFieldSchema(SamzaSqlFieldType fieldType, SqlFieldSchema elementType, SqlFieldSchema valueType, SqlSchema rowSchema) { | ||
| private SqlFieldSchema(SamzaSqlFieldType fieldType, SqlFieldSchema elementType, SqlFieldSchema valueType, | ||
| SqlSchema rowSchema, boolean isNullable, boolean hasDefaultValue) { |
There was a problem hiding this comment.
Do you think it makes sense to call the hasDefaultValue field as optionalField? I think it also is a good idea to document isNullable and isOptional in sqlFieldSchema to make the differences clear?
| RelRecordType outputRecord = (RelRecordType) QueryPlanner.getSourceRelSchema(relSchemaProvider, | ||
| new RelSchemaConverter()); | ||
| // Get Samza Sql schema along with Calcite schema. The reason is that the Calcite schema does not have a way | ||
| // to represent fields with default values while Samza Sql schema can represent default value fields. This is |
There was a problem hiding this comment.
Do you think it makes sense to call these as optional fields rather than fields with default values?
| SqlFieldSchema outputSqlFieldSchema = outputFieldSchemaMap.get(entry.getKey()); | ||
|
|
||
| if (projectFieldType == null) { | ||
| if (entry.getKey().equals(SamzaSqlRelMessage.KEY_NAME) || outputSqlFieldSchema.hasDefaultValue()) { |
There was a problem hiding this comment.
Can you add comments here on why we are special casing KEY_NAME field?
| } | ||
| } | ||
|
|
||
| // Ensure that all projected fields exist in the output schema and are of the same type. |
There was a problem hiding this comment.
nit: Rather than call them as projected fields can we say fields from sql statement?.
|
|
||
| if (outputFieldType == null) { | ||
| if (entry.getKey().equals(SamzaSqlRelMessage.OP_NAME)) { | ||
| if (entry.getKey().equals(SamzaSqlRelMessage.OP_NAME) || entry.getKey().equals(SamzaSqlRelMessage.KEY_NAME)) { |
There was a problem hiding this comment.
Can you add comments on why we are special casing these fields?
There was a problem hiding this comment.
Made key as optional and nullable value as it is by default being set to null in SamzaSqlRelMessage.
3 participants
No description provided.