Skip to content
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

Adds type conversions between PG types and arrow types #19

Closed
wants to merge 1 commit into from

Conversation

aykut-bozkurt
Copy link
Collaborator

These conversions will be needed when we convert PG types to arrow types and vice-versa.

let numeric: AnyNumeric = unsafe {
direct_function_call(
pg_sys::numeric_in,
&[numeric_str.into_datum(), 0.into_datum(), 0.into_datum()],
Copy link
Collaborator

@marcoslot marcoslot Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the 0 typemod seems a bit strange to me, since -1 is normally used as invalid typemod, but 0 kind of works by accident because it does not pass is_valid_numeric_typmod

I think it's probably ok to pass -1 here with a comment that we don't need numeric_in to validate the input.

let digit = (decimal % 10) as i8;
decimal_digits.push(digit);
decimal /= 10;
}
Copy link
Collaborator

@marcoslot marcoslot Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to construct the numeric directly without creating a string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably via writing as bytea and then calling numeric_recv, but string seems like a better idea to me

src/type_compat/pg_arrow_type_conversions.rs Outdated Show resolved Hide resolved
src/type_compat/pg_arrow_type_conversions.rs Outdated Show resolved Hide resolved
src/type_compat/pg_arrow_type_conversions.rs Outdated Show resolved Hide resolved
.expect("cannot adjust PG timestamptz to Unix timestamptz")
};

let adjusted_timestamptz_as_bytes: Vec<u8> = unsafe {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe:
// convert timestamptz to big endian bytes using timestamptz_send

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think expect part sounds like a comment.

let adjusted_timestamptz_as_bytes: Vec<u8> = unsafe {
        direct_function_call(
            pg_sys::timestamptz_send,
            &[adjusted_timestamptz.into_datum()],
        )
        .expect("cannot convert timestamptz to bytes")
    };


let sign = if numeric_str.starts_with('-') { -1 } else { 1 };

let numeric_str = numeric_str.replace(['-', '+', '.'], "");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use a comment on why we don't need to worry about where the fraction point is.

pub(crate) fn numeric_to_i128(numeric: AnyNumeric) -> i128 {
// obtain numeric's string representation
let numeric_str: &CStr = unsafe {
direct_function_call(pg_sys::numeric_out, &[numeric.into_datum()])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind liked that we were able to use send functions above. Is that not a possibility here? It seems at least the digits part looks vaguely similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it generates different byte representation than what parquet expects.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use a comment then

@aykut-bozkurt
Copy link
Collaborator Author

Addressed at development branch.

@aykut-bozkurt aykut-bozkurt deleted the crunchy_review branch October 17, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants