-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 uuid() function #4041
add uuid() function #4041
Conversation
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 code looks good to me. All that seems to be missing is a test
I wonder if we can run some basic sql level test - like select ... uuid() != uuid()
or something just to verify it is hooked up somehow
@@ -157,6 +157,7 @@ pub fn return_type( | |||
utf8_to_int_type(&input_expr_types[0], "octet_length") | |||
} | |||
BuiltinScalarFunction::Random => Ok(DataType::Float64), | |||
BuiltinScalarFunction::Uuid => Ok(DataType::Utf8), |
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 wonder if we want to try and return bytes (DataType::Binary
) instead of strings (and allow casting from bytes to string for anyone who wants strings) 🤔
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.
do you think binary is a better type than string? in this case the -
delimited hex is a standardized form however using binary people might confuse with 16 byte array...?
@@ -57,3 +57,4 @@ rand = "0.8" | |||
regex = { version = "^1.4.3", optional = true } | |||
sha2 = { version = "^0.10.1", optional = true } | |||
unicode-segmentation = { version = "^1.7.1", optional = true } | |||
uuid = { version = "^1.2", features = ["v4"] } |
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.
Moar dependencies 😢
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.
unlike the:
one i'd argue that uuid
is a bit ubiquitous so this should be fine
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 @jimexist
I still recommend a sql based test, but I think it is not strictly necessary
let sql = "SELECT uuid()"; | ||
let actual = execute(&ctx, sql).await; | ||
let uuid = actual[0][0].parse::<uuid::Uuid>().unwrap(); | ||
assert_eq!(uuid.get_version_num(), 4); |
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 @jimexist
Which issue does this PR close?
Closes #4045
Rationale for this change
add uuid() function
What changes are included in this PR?
Are there any user-facing changes?