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.

Sign up for GitHub

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

Encode column names with encoding of context #210

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+7 −8
Open

Encode column names with encoding of context #210

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ca5024
Encode column names with encoding of context
Adrian-Hirt Nov 21, 2024
9343cd4
Use `HAVE_RB_ENC_INTERNED_STR` instead of `HAVE_RB_INTERNED_STR` to c…
Adrian-Hirt Dec 5, 2024
b55afcf
Move `rb_to_encoding` call out of the loop
Adrian-Hirt Dec 5, 2024
70410b8
Store `rb_encoding` in `trilogy_ctx` struct
Adrian-Hirt Dec 5, 2024
88d6be0
Use `rb_encoding *` as type for connection encoding, remove obsolete …
Adrian-Hirt Dec 5, 2024
f84264c
Remove obsolete `have_func("rb_interned_str")` call
Adrian-Hirt Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions contrib/ruby/ext/trilogy-ruby/cext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,10 +811,12 @@ static VALUE read_query_response(VALUE vargs)
}
}

rb_encoding *conn_enc = rb_to_encoding(ctx->encoding);
Copy link
Contributor

Choose a reason for hiding this comment

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

rb_to_encoding is quite costly. We probably should it only once after connect or something, and keep a rb_encoding * in ctx.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the very least, it should be moved outside the loop, so that if you have 20 fields you don't do the conversion 20 times.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I moved the call to rb_to_encoding to the rb_trilogy_connect method and store it in rb_encoding *conn_encoding in the ctx. I also replaced the call to the rb_to_encoding method in rb_trilogy_query to use this value in the ctx struct. Let me know if you'd need any other adaptions for this.


#ifdef HAVE_RB_INTERNED_STR
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be updated to HAVE_RB_ENC_INTERNED_STR and extconf.rb should also be updated accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I changed this to use HAVE_RB_ENC_INTERNED_STR and added the corresponding have_func to extconf.rb

VALUE column_name = rb_interned_str(column.name, column.name_len);
VALUE column_name = rb_enc_interned_str(column.name, column.name_len, conn_enc);
#else
VALUE column_name = rb_str_new(column.name, column.name_len);
VALUE column_name = rb_enc_str_new(column.name, column.name_len, conn_enc);
OBJ_FREEZE(column_name);
#endif

Expand Down