You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/07/22 04:39:53 UTC
[GitHub] [arrow] pablosichert opened a new pull request #10772: ARROW-13426: [C++][Gandiva] Use platform pointer sizes in code generation
pablosichert opened a new pull request #10772:
URL: https://github.com/apache/arrow/pull/10772
Hey there!
This PR uses pointers in the JIT code generation / function invocation instead of the hard-coded `int64_t` type, which especially breaks when indexing into pointers to pointers on platforms where the pointer size is not 64 bit wide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10772: ARROW-13426: [C++][Gandiva] Use platform pointer sizes in code generation
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10772:
URL: https://github.com/apache/arrow/pull/10772#issuecomment-884651721
https://issues.apache.org/jira/browse/ARROW-13426
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pablosichert commented on a change in pull request #10772: ARROW-13426: [C++][Gandiva] Use platform pointer sizes in code generation
Posted by GitBox <gi...@apache.org>.
pablosichert commented on a change in pull request #10772:
URL: https://github.com/apache/arrow/pull/10772#discussion_r757956992
##########
File path: cpp/src/gandiva/llvm_types.h
##########
@@ -57,14 +58,20 @@ class GANDIVA_EXPORT LLVMTypes {
llvm::PointerType* ptr_type(llvm::Type* type) { return type->getPointerTo(); }
+ llvm::PointerType* opaque_ptr_type() { return ptr_type(i8_type()); }
Review comment:
That's because LLVM doesn't support opaque pointer types yet, see http://nondot.org/~sabre/LLVMNotes/EliminatingVoid.txt and https://llvm.org/docs/OpaquePointers.html.
Will add a comment here, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rkavanap commented on a change in pull request #10772: ARROW-13426: [C++][Gandiva] Use platform pointer sizes in code generation
Posted by GitBox <gi...@apache.org>.
rkavanap commented on a change in pull request #10772:
URL: https://github.com/apache/arrow/pull/10772#discussion_r757234336
##########
File path: cpp/src/gandiva/gdv_function_stubs.h
##########
@@ -55,119 +55,119 @@ using gdv_month_interval = int32_t;
#endif
#endif
-bool gdv_fn_like_utf8_utf8(int64_t ptr, const char* data, int data_len,
- const char* pattern, int pattern_len);
+bool gdv_fn_like_utf8_utf8(void* ptr, const char* data, int data_len, const char* pattern,
+ int pattern_len);
-bool gdv_fn_like_utf8_utf8_utf8(int64_t ptr, const char* data, int data_len,
+bool gdv_fn_like_utf8_utf8_utf8(void* ptr, const char* data, int data_len,
const char* pattern, int pattern_len,
const char* escape_char, int escape_char_len);
-bool gdv_fn_ilike_utf8_utf8(int64_t ptr, const char* data, int data_len,
+bool gdv_fn_ilike_utf8_utf8(void* ptr, const char* data, int data_len,
const char* pattern, int pattern_len);
-int64_t gdv_fn_to_date_utf8_utf8_int32(int64_t context, int64_t ptr, const char* data,
+int64_t gdv_fn_to_date_utf8_utf8_int32(void* context_ptr, void* ptr, const char* data,
int data_len, bool in1_validity,
const char* pattern, int pattern_len,
bool in2_validity, int32_t suppress_errors,
bool in3_validity, bool* out_valid);
-void gdv_fn_context_set_error_msg(int64_t context_ptr, const char* err_msg);
+void gdv_fn_context_set_error_msg(void* context_ptr, const char* err_msg);
-uint8_t* gdv_fn_context_arena_malloc(int64_t context_ptr, int32_t data_len);
+uint8_t* gdv_fn_context_arena_malloc(void* context_ptr, int32_t data_len);
-void gdv_fn_context_arena_reset(int64_t context_ptr);
+void gdv_fn_context_arena_reset(void* context_ptr);
-bool in_expr_lookup_int32(int64_t ptr, int32_t value, bool in_validity);
+bool in_expr_lookup_int32(void* ptr, int32_t value, bool in_validity);
-bool in_expr_lookup_int64(int64_t ptr, int64_t value, bool in_validity);
+bool in_expr_lookup_int64(void* ptr, int64_t value, bool in_validity);
-bool in_expr_lookup_utf8(int64_t ptr, const char* data, int data_len, bool in_validity);
+bool in_expr_lookup_utf8(void* ptr, const char* data, int data_len, bool in_validity);
int gdv_fn_time_with_zone(int* time_fields, const char* zone, int zone_len,
int64_t* ret_time);
GANDIVA_EXPORT
-const char* gdv_fn_base64_encode_binary(int64_t context, const char* in, int32_t in_len,
+const char* gdv_fn_base64_encode_binary(void* context_ptr, const char* in, int32_t in_len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_base64_decode_utf8(int64_t context, const char* in, int32_t in_len,
+const char* gdv_fn_base64_decode_utf8(void* context_ptr, const char* in, int32_t in_len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_castVARBINARY_int32_int64(int64_t context, gdv_int32 value,
+const char* gdv_fn_castVARBINARY_int32_int64(void* context_ptr, gdv_int32 value,
int64_t out_len, int32_t* out_length);
GANDIVA_EXPORT
-const char* gdv_fn_castVARBINARY_int64_int64(int64_t context, gdv_int64 value,
+const char* gdv_fn_castVARBINARY_int64_int64(void* context_ptr, gdv_int64 value,
int64_t out_len, int32_t* out_length);
GANDIVA_EXPORT
-const char* gdv_fn_sha256_decimal128(int64_t context, int64_t x_high, uint64_t x_low,
+const char* gdv_fn_sha256_decimal128(void* context_ptr, int64_t x_high, uint64_t x_low,
int32_t x_precision, int32_t x_scale,
gdv_boolean x_isvalid, int32_t* out_length);
GANDIVA_EXPORT
-const char* gdv_fn_sha1_decimal128(int64_t context, int64_t x_high, uint64_t x_low,
+const char* gdv_fn_sha1_decimal128(void* context_ptr, int64_t x_high, uint64_t x_low,
int32_t x_precision, int32_t x_scale,
gdv_boolean x_isvalid, int32_t* out_length);
-int32_t gdv_fn_dec_from_string(int64_t context, const char* in, int32_t in_length,
+int32_t gdv_fn_dec_from_string(void* context_ptr, const char* in, int32_t in_length,
int32_t* precision_from_str, int32_t* scale_from_str,
int64_t* dec_high_from_str, uint64_t* dec_low_from_str);
-char* gdv_fn_dec_to_string(int64_t context, int64_t x_high, uint64_t x_low,
+char* gdv_fn_dec_to_string(void* context_ptr, int64_t x_high, uint64_t x_low,
int32_t x_scale, int32_t* dec_str_len);
GANDIVA_EXPORT
-int32_t gdv_fn_castINT_utf8(int64_t context, const char* data, int32_t data_len);
+int32_t gdv_fn_castINT_utf8(void* context_ptr, const char* data, int32_t data_len);
GANDIVA_EXPORT
-int64_t gdv_fn_castBIGINT_utf8(int64_t context, const char* data, int32_t data_len);
+int64_t gdv_fn_castBIGINT_utf8(void* context_ptr, const char* data, int32_t data_len);
GANDIVA_EXPORT
-float gdv_fn_castFLOAT4_utf8(int64_t context, const char* data, int32_t data_len);
+float gdv_fn_castFLOAT4_utf8(void* context_ptr, const char* data, int32_t data_len);
GANDIVA_EXPORT
-double gdv_fn_castFLOAT8_utf8(int64_t context, const char* data, int32_t data_len);
+double gdv_fn_castFLOAT8_utf8(void* context_ptr, const char* data, int32_t data_len);
GANDIVA_EXPORT
-const char* gdv_fn_castVARCHAR_int32_int64(int64_t context, int32_t value, int64_t len,
+const char* gdv_fn_castVARCHAR_int32_int64(void* context_ptr, int32_t value, int64_t len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_castVARCHAR_int64_int64(int64_t context, int64_t value, int64_t len,
+const char* gdv_fn_castVARCHAR_int64_int64(void* context_ptr, int64_t value, int64_t len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_castVARCHAR_float32_int64(int64_t context, float value, int64_t len,
+const char* gdv_fn_castVARCHAR_float32_int64(void* context_ptr, float value, int64_t len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_castVARCHAR_float64_int64(int64_t context, double value, int64_t len,
+const char* gdv_fn_castVARCHAR_float64_int64(void* context_ptr, double value, int64_t len,
int32_t* out_len);
GANDIVA_EXPORT
int32_t gdv_fn_utf8_char_length(char c);
GANDIVA_EXPORT
-const char* gdv_fn_upper_utf8(int64_t context, const char* data, int32_t data_len,
+const char* gdv_fn_upper_utf8(void* context_ptr, const char* data, int32_t data_len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_lower_utf8(int64_t context, const char* data, int32_t data_len,
+const char* gdv_fn_lower_utf8(void* context_ptr, const char* data, int32_t data_len,
int32_t* out_len);
GANDIVA_EXPORT
-const char* gdv_fn_initcap_utf8(int64_t context, const char* data, int32_t data_len,
+const char* gdv_fn_initcap_utf8(void* context_ptr, const char* data, int32_t data_len,
int32_t* out_len);
GANDIVA_EXPORT
-int32_t gdv_fn_castINT_varbinary(gdv_int64 context, const char* in, int32_t in_len);
Review comment:
why was this gdv_int64 instead of int64_t earlier? guessing they are of the same type.
##########
File path: cpp/src/gandiva/precompiled/string_ops.cc
##########
@@ -561,111 +564,112 @@ gdv_boolean castBIT_utf8(gdv_int64 context, const char* data, gdv_int32 data_len
if (compare_lower_strings("false", 5, trimmed_data, trimmed_len)) return false;
}
// if no 'true', 'false', '0' or '1' value is found, set an error
- gdv_fn_context_set_error_msg(context, "Invalid value for boolean.");
+ gdv_fn_context_set_error_msg(context_ptr, "Invalid value for boolean.");
Review comment:
actually this is a comment not related to this PR. But I feel the error message should have more detail. For instance, 'Invalid value for boolean \<value\>' Just simply stating Invalid value for boolean does not give clarity on what was wrong with the value.
##########
File path: cpp/src/gandiva/llvm_types.h
##########
@@ -57,14 +58,20 @@ class GANDIVA_EXPORT LLVMTypes {
llvm::PointerType* ptr_type(llvm::Type* type) { return type->getPointerTo(); }
+ llvm::PointerType* opaque_ptr_type() { return ptr_type(i8_type()); }
Review comment:
shouldn't opaque_ptr_type be ptr_type(void_type)?; any reason why it is ptr_type() of i8_type
##########
File path: cpp/src/gandiva/precompiled/string_ops.cc
##########
@@ -530,9 +533,9 @@ gdv_boolean compare_lower_strings(const char* base_str, gdv_int32 base_str_len,
// Try to cast the received string ('0', '1', 'true', 'false'), ignoring leading
// and trailing spaces, also ignoring lower and upper case.
FORCE_INLINE
-gdv_boolean castBIT_utf8(gdv_int64 context, const char* data, gdv_int32 data_len) {
+gdv_boolean castBIT_utf8(void* context_ptr, const char* data, gdv_int32 data_len) {
if (data_len <= 0) {
- gdv_fn_context_set_error_msg(context, "Invalid value for boolean.");
+ gdv_fn_context_set_error_msg(context_ptr, "Invalid value for boolean.");
Review comment:
Not related to this PR. But it is a generic comment on not enough information in error messages through out Gandiva, which make it very difficult to understand why Gandiva rejected a request.
In this case, the error message should have more details: e.g 'Invalid value for boolean; bad length \<length\>'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org