-
Notifications
You must be signed in to change notification settings - Fork 281
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 support for numerics #1324
add support for numerics #1324
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.
Just a quick look, but this code is generally structured the way you gotta do it. It always feels wrong to enumerate a hardcoded list of type oids/names, but that's just kinda the nature of the beast with Rust and Postgres.
"int8" => row[column.position].value::<i64>().unwrap().map(|v| v.to_string()), | ||
"float4" => row[column.position].value::<f32>().unwrap().map(|v| v.to_string()), | ||
"float8" => row[column.position].value::<f64>().unwrap().map(|v| v.to_string()), | ||
"numeric" => row[column.position].value::<AnyNumeric>().unwrap().map(|v| v.to_string()), | ||
"bpchar" | "text" | "varchar" => { |
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.
Hey! @montanalow asked me to take a quick look here.
I feel like these string types names would be better as OID values. pgrx has them all in pg_sys::
, like pg_sys::INT8OID
, pg_sys::NUMERICOID
, pg_sys::NUMERICARRAYOID
, and so on. It looks like this is the approach you use up in model.rs
already.
I guess you'd have to figure out the type oids wherever you construct the Column
entries, but that doesn't seem too difficult. I think in that SELECT statement around line 505 you could write udt_name::text::regtype::oid
instead of udt_name::TEXT
.
Comparing on Oid value will be a little more performant, I suppose, and it'll future proof you from accidentally making type-os in the code.
_ => error!( | ||
"Unhandled type for quantitative scalar column: {} {:?}", | ||
column.name, column.pg_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.
And now that you have the type oids, you could call its output function here and just get its string representation from Postgres. Who knows if it'd then be something you could otherwise handle, but maybe?
Same thing around line 1090 for the array case.
fix #1041