Merge pull request #1 from mschrader15/parquet-debug

Segmentation Fault Fixes

src/CMakeLists.txt

@@ -1,7 +1,9 @@
 set(netconvertlibs
         netwrite netimport netbuild foreign_eulerspiral ${GDAL_LIBRARY} netimport_vissim netimport_vissim_typeloader netimport_vissim_tempstructs ${commonlibs} ${TCMALLOC_LIBRARY})
 
-FIND_LIBRARY(PARQUET_LIBRARY NAMES parquet)
+if(WITH_PARQUET)
+    FIND_LIBRARY(PARQUET_LIBRARY NAMES parquet)
+endif()
 
 set(sumolibs
         traciserver netload microsim_cfmodels microsim_engine microsim_lcmodels microsim_devices microsim_trigger microsim_output microsim_transportables microsim_actions

src/utils/iodevices/OutputDevice.cpp

@@ -226,12 +226,6 @@ OutputDevice::OutputDevice(const std::string& filename,  OutputFormatter* format
 }
 
 
-
-OutputDevice::~OutputDevice() {
-    delete myFormatter;
-}
-
-
 bool
 OutputDevice::ok() {
     return getOStream().good();

src/utils/iodevices/OutputDevice.h

@@ -153,7 +153,7 @@ class OutputDevice {
 
 
     /// @brief Destructor
-    virtual ~OutputDevice();
+    virtual ~OutputDevice() = default;
 
 
     /** @brief returns the information whether one can write into the device
@@ -209,7 +209,7 @@ class OutputDevice {
 
     template <typename E>
     bool writeHeader(const SumoXMLTag& rootElement) {
-        return getFormatter()->writeHeader(getOStream(), rootElement);
+        return getFormatter().writeHeader(getOStream(), rootElement);
     }
 
 
@@ -308,12 +308,12 @@ class OutputDevice {
         {
         case OutputWriterType::XML:
             // cast the writer to the correct type
-            getFormatter<PlainXMLFormatter>()->writeAttr(getOStream(), attr, val);
+            getFormatter<PlainXMLFormatter>().writeAttr(getOStream(), attr, val);
             break;
         case OutputWriterType::PARQUET:
 #ifdef HAVE_PARQUET
             // cast the writer to the correct type
-            getFormatter<ParquetFormatter>()->writeAttr(getOStream(), attr, val);
+            getFormatter<ParquetFormatter>().writeAttr(getOStream(), attr, val);
 #else
             throw IOError("Parquet output is not supported in this build. Please recompile with the correct options.");
 #endif
@@ -372,7 +372,7 @@ class OutputDevice {
         // getOStream() << t;
         // get the correct formatter
         if (this->getOStream().allowRaw()) {
-            this->getOStream() << t;
+            writeRaw(t);
         }
         else {
             throw IOError("Raw output is not allowed for this output device");
@@ -395,6 +395,18 @@ class OutputDevice {
         getOStream().setOSFlags(flags);
     }
 
+    // @brief handle the raw write
+    template <typename T>
+    void writeRaw(const T& val) {
+        // cast the writer to the correct type
+        if (this->getType() == OutputWriterType::XML) {
+            getFormatter<PlainXMLFormatter>().writeRaw(getOStream(), val);
+        }
+        else {
+            throw IOError("Raw output is not supported for this output type");
+        }
+    }
+
 protected:
     /// @brief Returns the associated ostream
     virtual StreamDevice& getOStream() {
@@ -418,8 +430,8 @@ class OutputDevice {
     virtual void postWriteHook();
 
     /// @brief Returns the formatter
-    OutputFormatter* getFormatter() {
-        return myFormatter;
+    OutputFormatter& getFormatter() {
+        return *myFormatter;
     }
 
 
@@ -434,15 +446,15 @@ class OutputDevice {
     const std::string myFilename;
 
     /// @brief the stream device
-    StreamDevice* myStreamDevice;
+    std::unique_ptr<StreamDevice> myStreamDevice{nullptr};
 
     /// @brief The formatter for XML
-    OutputFormatter* myFormatter;
+    std::unique_ptr<OutputFormatter> myFormatter{nullptr};
 
     /// @brief return a type casted formatter
     template <typename T>
-    T* getFormatter() {
-        return static_cast<T*>(myFormatter);
+    T& getFormatter() {
+        return static_cast<T&>(*myFormatter);
     }
 
 private:

src/utils/iodevices/OutputDevice_CERR.cpp

@@ -47,7 +47,7 @@ OutputDevice_CERR::getDevice() {
 // method definitions
 // ===========================================================================
 OutputDevice_CERR::OutputDevice_CERR() : OutputDevice(0, "CERR") {
-    myStreamDevice = new OStreamDevice(&std::cerr);
+    myStreamDevice = std::make_unique<COUTStreamDevice>(std::cerr);
 }
 
 

src/utils/iodevices/OutputDevice_COUT.cpp

@@ -47,7 +47,7 @@ OutputDevice_COUT::getDevice() {
 // method definitions
 // ===========================================================================
 OutputDevice_COUT::OutputDevice_COUT() : OutputDevice(0, "COUT") {
-    myStreamDevice = new OStreamDevice(&std::cout);
+    myStreamDevice = std::make_unique<COUTStreamDevice>();
 }
 
 

src/utils/iodevices/OutputDevice_File.cpp

@@ -40,7 +40,7 @@ OutputDevice_File::OutputDevice_File(const std::string& fullName, const bool com
     if (fullName == "/dev/null") {
         myAmNull = true;
 #ifdef WIN32
-        myStreamDevice = new OStreamDevice(new std::ofstream("NUL"));
+        myStreamDevice = std::make_unique<OStreamDevice>(new std::ofstream("NUL"));
         if (!myFileStream->good()) {
             delete myFileStream;
             throw IOError(TLF("Could not redirect to NUL device (%).", std::string(std::strerror(errno))));
@@ -52,28 +52,22 @@ OutputDevice_File::OutputDevice_File(const std::string& fullName, const bool com
 #ifdef HAVE_ZLIB
     if (compressed) {
         try {
-            myStreamDevice = new OStreamDevice(new zstr::ofstream(localName.c_str(), std::ios_base::out));
+            myStreamDevice = std::make_unique<OStreamDevice>(new zstr::ofstream(localName.c_str(), std::ios_base::out));
         } catch (strict_fstream::Exception& e) {
             throw IOError("Could not build output file '" + fullName + "' (" + e.what() + ").");
         } catch (zstr::Exception& e) {
             throw IOError("Could not build output file '" + fullName + "' (" + e.what() + ").");
         }
     } else {
-        myStreamDevice = new OStreamDevice(new std::ofstream(localName.c_str(), std::ios_base::out));
+        myStreamDevice = std::make_unique<OStreamDevice>(new std::ofstream(localName.c_str(), std::ios_base::out));
     }
 #else
     UNUSED_PARAMETER(compressed);
-    myFileStream = new std::ofstream(localName.c_str(), std::ios_base::out);
+    myStreamDevice = std::make_unique<OStreamDevice>(new std::ofstream(localName.c_str(), std::ios_base::out));
 #endif
     if (!myStreamDevice->good()) {
-        delete myStreamDevice;
+        myStreamDevice.reset();
         throw IOError("Could not build output file '" + fullName + "' (" + std::strerror(errno) + ").");
     }
 }
-
-
-OutputDevice_File::~OutputDevice_File() {
-    delete myStreamDevice;
-}
-
 /****************************************************************************/

src/utils/iodevices/OutputDevice_File.h

@@ -45,10 +45,6 @@ class OutputDevice_File : public OutputDevice {
      */
     OutputDevice_File(const std::string& fullName, const bool compressed = false);
 
-
-    /// @brief Destructor
-    ~OutputDevice_File();
-
     /** @brief returns the information whether the device will discard all output
      * @return Whether the device redirects to /dev/null
      */

src/utils/iodevices/OutputDevice_Parquet.cpp

@@ -53,17 +53,26 @@ OutputDevice_Parquet::OutputDevice_Parquet(const std::string& fullName)
 
 
 bool OutputDevice_Parquet::closeTag(const std::string& comment) {
-    // open the file for writing
+    UNUSED_PARAMETER(comment);
+    // open the file for writing, but only if the depth is >=2 (i.e. we are closing the children tag).
+    //! @todo this is a bit of a hack, but it works for now
+    auto formatter = dynamic_cast<ParquetFormatter*>(&this->getFormatter());
+    if (formatter->getDepth() < 2) {
+        // we have to clean up the stack, otherwise the file will not be written correctly
+        // when it is open
+        formatter->clearStack();
+        // this is critical for the file to be written correctly
+        return false;
+    }
     if (myFile == nullptr) {
-        auto formatter = dynamic_cast<ParquetFormatter*>(this->getFormatter());
         if (formatter == nullptr) {
             throw IOError("Formatter is not a ParquetFormatter");
         }
         // Create a Parquet file
         PARQUET_ASSIGN_OR_THROW(
             this->myFile, arrow::io::FileOutputStream::Open(this->myFilename));
 
-        this->myStreamDevice = new ParquetStream(parquet::ParquetFileWriter::Open(this->myFile, std::static_pointer_cast<parquet::schema::GroupNode>(
+        this->myStreamDevice = std::make_unique<ParquetStream>(parquet::ParquetFileWriter::Open(this->myFile, std::static_pointer_cast<parquet::schema::GroupNode>(
             parquet::schema::GroupNode::Make("schema", parquet::Repetition::REQUIRED, formatter->getNodeVector())
         ), this->builder.build()));
 
@@ -73,15 +82,18 @@ bool OutputDevice_Parquet::closeTag(const std::string& comment) {
         }
     }
     // now actually write the data
-    return getFormatter()->closeTag(getOStream());
+    return formatter->closeTag(getOStream());
 }
 
 
 OutputDevice_Parquet::~OutputDevice_Parquet() {
     // have to delete the stream device before the file. This dumps unwritten data to the file
-    delete myStreamDevice;
+    myStreamDevice.reset();
+    // close the file (if open)
+    if (this->myFile.get() == nullptr) {
+        return;
+    }
     [[maybe_unused]] arrow::Status status = this->myFile->Close();
-
 }
 
 #endif

src/utils/iodevices/OutputDevice_Parquet.h

@@ -39,23 +39,22 @@
 
 
 /**
- * @class OutputDevice_File
- * @brief An output device that encapsulates an ofstream
+ * @class OutputDevice_Parquet
+ * @brief An output device that encapsulates an parquet stream writer
  *
- * Please note that the device gots responsible for the stream and deletes
+ * Please note that the device is responsible for the stream and deletes
  *  it (it should not be deleted elsewhere).
  */
 class OutputDevice_Parquet : public OutputDevice {
 public:
     /** @brief Constructor
      * @param[in] fullName The name of the output file to use
-     * @param[in] compressed whether to apply gzip compression
      * @exception IOError Should not be thrown by this implementation
      */
     OutputDevice_Parquet(const std::string& fullName);
 
     /// @brief Destructor
-    ~OutputDevice_Parquet();
+    ~OutputDevice_Parquet() override;
 
     /** @brief implements the close tag logic. This is where the file is first opened and the   schema is created.
      * This exploits the fact that for *most* SUMO files, all the fields are present at the first close tag event.
@@ -67,10 +66,12 @@ class OutputDevice_Parquet : public OutputDevice {
     void lf() {};
 
     // null the setPrecision method
-    void setPrecision(int precision) override {};
+    void setPrecision(int precision) override {
+        UNUSED_PARAMETER(precision);
+    };
 
     void setOSFlags(std::ios_base::fmtflags flags) override {
-
+        UNUSED_PARAMETER(flags);
     };
 
 protected:

src/utils/iodevices/OutputDevice_String.cpp

@@ -29,17 +29,11 @@
 // ===========================================================================
 OutputDevice_String::OutputDevice_String(const int defaultIndentation)
     : OutputDevice(defaultIndentation) {
-    auto stream = new std::ostringstream();
-    (*stream) << std::setiosflags(std::ios::fixed);
-    myStreamDevice = new OStreamDevice(stream);
-    setPrecision();
+    myStreamDevice = std::make_unique<OStreamDevice>(new std::ostringstream());
+    myStreamDevice->setOSFlags(std::ios::fixed);
+    myStreamDevice->setPrecision(2);
 }
 
-
-OutputDevice_String::~OutputDevice_String() {
-}
-
-
 std::string
 OutputDevice_String::getString() const {
     return myStreamDevice->str();

src/utils/iodevices/OutputDevice_String.h

@@ -41,11 +41,6 @@ class OutputDevice_String : public OutputDevice {
      */
     OutputDevice_String(const int defaultIndentation = 0);
 
-
-    /// @brief Destructor
-    ~OutputDevice_String();
-
-
     /** @brief Returns the current content as a string
      * @return The content as string
      */

src/utils/iodevices/OutputFormatter.h

@@ -50,7 +50,7 @@ class RGBColor;
 class OutputFormatter {
 public:
     /// @brief Destructor
-    virtual ~OutputFormatter() { }
+    virtual ~OutputFormatter() = default;
 
 
     /** @brief Writes an XML header with optional configuration