Various review changes.

export/planloader/CMakeLists.txt

@@ -1,7 +1,5 @@
 # SPDX-License-Identifier: Apache-2.0
 
-set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)
-
 if(NOT BUILD_SUBDIR_NAME EQUAL "release")
  message(
  SEND_ERROR,
@@ -10,7 +8,6 @@ if(NOT BUILD_SUBDIR_NAME EQUAL "release")
 endif()
 
 add_library(planloader SHARED planloader.cpp)
-set_target_properties(planloader PROPERTIES SUBVERSION 1)
 
 add_dependencies(planloader substrait_io)
 target_link_libraries(planloader substrait_io)

export/planloader/planloader.cpp

@@ -10,23 +10,23 @@ extern "C" {
 // Load a Substrait plan (in any format) from disk.
 // Stores the Substrait plan in planBuffer in serialized form.
 // Returns a SerializedPlan structure containing either the serialized plan or
-// an error message. errorMessage is nullptr upon success.
+// an error message. error_message is nullptr upon success.
 SerializedPlan* load_substrait_plan(const char* filename) {
  auto newPlan = new SerializedPlan();
  newPlan->buffer = nullptr;
  newPlan->size = 0;
- newPlan->errorMessage = nullptr;
+ newPlan->error_message = nullptr;
 
  auto planOrError = io::substrait::loadPlan(filename);
  if (!planOrError.ok()) {
  auto errMsg = planOrError.status().message();
- newPlan->errorMessage = new char[errMsg.length()+1];
- strncpy(newPlan->errorMessage, errMsg.data(), errMsg.length()+1);
+ newPlan->error_message = new char[errMsg.length()+1];
+ strncpy(newPlan->error_message, errMsg.data(), errMsg.length()+1);
  return newPlan;
  }
  ::substrait::proto::Plan plan = *planOrError;
  std::string text = plan.SerializeAsString();
- newPlan->buffer = new char[text.length()+1];
+ newPlan->buffer = new unsigned char[text.length()+1];
  memcpy(newPlan->buffer, text.data(), text.length()+1);
  newPlan->size = static_cast<int32_t>(
  text.length() &
@@ -36,20 +36,20 @@ SerializedPlan* load_substrait_plan(const char* filename) {
 
 void free_substrait_plan(SerializedPlan* plan) {
  delete[] plan->buffer;
- delete[] plan->errorMessage;
+ delete[] plan->error_message;
  delete plan;
 }
 
 // Write a serialized Substrait plan to disk in the specified format.
 // On error returns a non-empty error message.
 // On success a nullptr is returned.
 const char* save_substrait_plan(
- const char* planData,
- uint32_t planDataLength,
+ const unsigned char* plan_data,
+ uint32_t plan_data_length,
  const char* filename,
  io::substrait::PlanFileFormat format) {
  ::substrait::proto::Plan plan;
- std::string data(planData, planDataLength);
+ std::string data((const char*) plan_data, plan_data_length);
  plan.ParseFromString(data);
  auto result = io::substrait::savePlan(plan, filename, format);
  if (!result.ok()) {

export/planloader/planloader.h

@@ -6,31 +6,38 @@ extern "C" {
 
 using SerializedPlan = struct {
  // If set, contains a serialized ::substrait::proto::Plan object.
- char *buffer;
+ unsigned char *buffer;
  // If buffer is set, this is the size of the buffer.
  int32_t size;
  // If null the buffer is valid, otherwise this points to a null terminated
  // error string.
- char *errorMessage;
+ char *error_message;
 };
 
 // Since this is actually C code, stick to C style names for exporting.
 // NOLINTBEGIN(readability-identifier-naming)
 
 // Load a Substrait plan (in any format) from disk.
-// Stores the Substrait plan in planBuffer in serialized form.
+//
+// Accepts filename as a null-terminated C string.
 // Returns a SerializedPlan structure containing either the serialized plan or
-// an error message.
+// an error message. This SerializedPlan should be freed using
+// free_substrait_plan.
 SerializedPlan* load_substrait_plan(const char* filename);
 
+// Frees a SerializedPlan that was returned from load_substrait_plan.
 void free_substrait_plan(SerializedPlan* plan);
 
 // Write a serialized Substrait plan to disk in the specified format.
+//
+// plan_data is a Substrait Plan serialized into a byte array with length
+// plan_data_length.
+// Filename is a null-terminated C string.
 // On error returns a non-empty error message.
 // On success an empty string is returned.
 const char* save_substrait_plan(
- const char* planData,
- uint32_t planDataLength,
+ const unsigned char* plan_data,
+ uint32_t plan_data_length,
  const char* filename,
  io::substrait::PlanFileFormat format);
 

export/planloader/tests/PlanLoaderTest.cpp

@@ -11,7 +11,7 @@ namespace {
 
 TEST(PlanLoaderTest, LoadAndSave) {
  auto serializedPlan = load_substrait_plan("data/q6_first_stage.json");
- ASSERT_EQ(serializedPlan->errorMessage, nullptr);
+ ASSERT_EQ(serializedPlan->error_message, nullptr);
 
  ::substrait::proto::Plan plan;
  bool parseStatus =