-
Notifications
You must be signed in to change notification settings - Fork 187
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
Add EXTRACT_COLLECTION UDF to registry #533
Conversation
@@ -681,6 +681,8 @@ public boolean isOptional(int i) { | |||
SqlTypeFamily.BOOLEAN, SqlTypeFamily.ANY, SqlTypeFamily.ARRAY, SqlTypeFamily.STRING, SqlTypeFamily.STRING)); | |||
createAddUserDefinedFunction("com.linkedin.groot.runtime.udf.spark.GetMappedValueUDF", FunctionReturnTypes.STRING, | |||
family(SqlTypeFamily.STRING, SqlTypeFamily.STRING)); | |||
createAddUserDefinedFunction("com.linkedin.groot.runtime.udf.spark.ExtractCollectionUDF", | |||
FunctionReturnTypes.arrayOfType(SqlTypeName.ANY, true), family(SqlTypeFamily.ANY, SqlTypeFamily.STRING)); |
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.
return type shouldn't contain SqlTypeName.ANY
, is this the same type as the first parameter's type?
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.
@ljfgem I discussed with Qiang offline, this UDF's return type is pretty complicated, it depends on its input arg type via some complex logic, and I'm not sure if it's even possible to express all that type in calcite. So I think using a broader arrayOfType of ANY type to express it should be fine here? I understand it will be a wider type than what this function strictly is, but it shouldn't cause any problems as long as the user of this udf knows what it's doing and uses it correctly with expected operand types.
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.
IIRC, we encountered a similar issue while generating Avro schema for the UDF field because RelDataTypeToAvroType#basicSqlTypeToAvroType
doesn't support ANY
type and would throw exception.
If coral-schema fails, the UDF couldn't be registered properly and Spark query would fail.
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.
The inputs are 1. an array field in dataframe which array contains struct and potentially nested arrays. 2. a string path reference from (1) to a primitive leaf field (int, long, string etc).
The return type will be an array of the type of leaf field, which type is only available at runtime
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.
@ljfgem If ANY is not possible, is there any way to set it to a union type? Something like Array<Int, Long and String>
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.
Changed to FunctionReturnTypes.arrayOfType(SqlTypeName.VARCHAR, true)
Please review again. Thanks
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.
Entry looks fine to me
What changes are proposed in this pull request, and why are they necessary?
Added a new UDF to static Hive function registry
How was this patch tested?
./gradlew clean build