-
Notifications
You must be signed in to change notification settings - Fork 66
feat: add SortOrder::Make and SortOrder::Validate #300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Junwang Zhao <[email protected]>
wgtmac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I have left some minor comments.
| if (schema_field == std::nullopt) { | ||
| return InvalidArgument("Cannot find schema field for sort field: {}", field); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (schema_field == std::nullopt) { | |
| return InvalidArgument("Cannot find schema field for sort field: {}", field); | |
| } | |
| if (schema_field == std::nullopt) { | |
| continue; | |
| } |
I think we should not error out if the field is missing. The Java impl is as below:
public SortOrder bind(Schema schema) {
SortOrder.Builder builder = SortOrder.builderFor(schema).withOrderId(orderId);
for (UnboundSortField field : fields) {
Type sourceType = schema.findType(field.sourceId);
Transform<?, ?> transform;
if (sourceType != null) {
transform = Transforms.fromString(sourceType, field.transform.toString());
} else {
transform = field.transform;
}
builder.addSortField(transform, field.sourceId, field.direction, field.nullOrder);
}
return builder.build();
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, UnboundSortOrder doesn't check, but checkCompatibility should do the check, see [1].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
| /// \brief Checks whether this function can be applied to the given Type. | ||
| /// \param source_type The source type to check. | ||
| /// \return true if this transform can be applied to the type, false otherwise | ||
| bool CanTransform(const Type& source_type) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider changing the parameter to const std::shared_ptr& for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a const ref of Type here should be fine, the Bind function takes a const shared_ptr& because the returned TransformFunction holds its own shared_ptr.
I'm open to using const shared_ptr& if more people think it's the better approach.
No description provided.