ARROW-13426: [C++][Gandiva] Use platform pointer sizes in code generation #10772
Conversation
Signed-off-by: Pablo Sichert <mail@pablosichert.com>
Signed-off-by: Pablo Sichert <mail@pablosichert.com>
…64_t* Signed-off-by: Pablo Sichert <mail@pablosichert.com>
Signed-off-by: Pablo Sichert <mail@pablosichert.com>
pablosichert
force-pushed
the
gandiva-code-generation-platform-pointer-size
branch
from
90e2109 to
835bd27
Compare
| int32_t* out_len); | ||
|
|
||
| GANDIVA_EXPORT | ||
| int32_t gdv_fn_castINT_varbinary(gdv_int64 context, const char* in, int32_t in_len); |
There was a problem hiding this comment.
why was this gdv_int64 instead of int64_t earlier? guessing they are of the same type.
| @@ -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()); } | |||
There was a problem hiding this comment.
shouldn't opaque_ptr_type be ptr_type(void_type)?; any reason why it is ptr_type() of i8_type
There was a problem hiding this 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.
There was a problem hiding this comment.
We've recently added support for opaque pointers
| @@ -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."); | |||
There was a problem hiding this 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.
| 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."); |
There was a problem hiding this 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>'
|
Is it worth keeping this open/reviving it? cc @js8544 |
This feature is definitely worth having, if the author would like to resolve the conflicts. |
|
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍 |
Hey there!
This PR uses pointers in the JIT code generation / function invocation instead of the hard-coded
int64_ttype, which especially breaks when indexing into pointers to pointers on platforms where the pointer size is not 64 bit wide.