SAMZA-2375, SAMZA-2376: Add display name to UdfMetadata and handle trailing semi-colons #1214
Conversation
| @@ -32,6 +32,7 @@ | |||
| public class UdfMetadata { | |||
|
|
|||
| private final String name; | |||
| private final String displayName; | |||
There was a problem hiding this comment.
Might want to add a comment/example explaining the difference between name and displayName (if this isn't a well know samza concept)?
| @@ -32,6 +32,7 @@ | |||
| public class UdfMetadata { | |||
|
|
|||
| private final String name; | |||
There was a problem hiding this comment.
Might be better to rename the field name to something more meaningful(like udfLogicalName or something else).
There was a problem hiding this comment.
Added a description of what this field means. Well, it is just udf name. For UdfMetadata, it should not matter whether it is logicalName or a physicalName, it is just the name that the udf will be referred as. I feel name is appropriate. What do you think ?
| @@ -45,6 +46,8 @@ public UdfMetadata(String name, String description, Method udfMethod, Config udf | |||
| SamzaSqlFieldType returnType, boolean disableArgCheck) { | |||
| // Udfs are case insensitive | |||
| this.name = name.toUpperCase(); | |||
| // Let's also store the original name for display purposes. | |||
| this.displayName = name; | |||
There was a problem hiding this comment.
This POJO already has hashcode and equals implemented in it. Are we planning to account for this new field in them?
There was a problem hiding this comment.
It should be ok not to account for the new field as name is already accounted for and we expect udfs to be anyways case insensitive.
| @@ -101,6 +101,8 @@ public String getSql() { | |||
| public static QueryInfo parseQuery(String sql) { | |||
| Planner planner = createPlanner(); | |||
| SqlNode sqlNode; | |||
| // Remove trailing semi-colons | |||
| sql = sql.replaceAll(";+$", ""); | |||
There was a problem hiding this comment.
Would be great if we can extract the regex to match the semi-colon into a appropriately named constant to improve readability. Also, if possible, we can enrich the comment as-to why the removal was required.
No description provided.