SNOW-991012 Cleanup some C++ code warnings and performance issues (#1828)

Co-authored-by: Mark Keller <[email protected]>

Author: sfc-gh-llorimer

DESCRIPTION.md

@@ -11,6 +11,7 @@ Source code is also available at: https://github.com/snowflakedb/snowflake-conne
 - v3.7.0(TBD)
 
   - Added a new boolean parameter `force_return_table` to `SnowflakeCursor.fetch_arrow_all` to force returning `pyarrow.Table` in case of zero rows.
+  - Cleanup some C++ code warnings and performance issues.
 
 - v3.6.0(December 09,2023)
 

setup.py

@@ -75,6 +75,8 @@ def build_extension(self, ext):
             if options["debug"]:
                 ext.extra_compile_args.append("-g")
                 ext.extra_link_args.append("-g")
+                ext.extra_compile_args.append("-O0")
+                ext.extra_link_args.append("-O0")
             current_dir = os.getcwd()
 
             if ext.name == "snowflake.connector.nanoarrow_arrow_iterator":

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/CArrowChunkIterator.cpp

@@ -40,15 +40,15 @@ CArrowChunkIterator::CArrowChunkIterator(PyObject* context, char* arrow_bytes, i
                m_columnCount, m_useNumpy);
 }
 
-std::shared_ptr<ReturnVal> CArrowChunkIterator::next()
+ReturnVal CArrowChunkIterator::next()
 {
   m_rowIndexInBatch++;
 
   if (m_rowIndexInBatch < m_rowCountInBatch)
   {
     this->createRowPyObject();
     SF_CHECK_PYTHON_ERR()
-    return std::make_shared<ReturnVal>(m_latestReturnedRow.get(), nullptr);
+    return ReturnVal(m_latestReturnedRow.get(), nullptr);
   }
   else
   {
@@ -69,13 +69,13 @@ std::shared_ptr<ReturnVal> CArrowChunkIterator::next()
       this->createRowPyObject();
       SF_CHECK_PYTHON_ERR()
 
-      return std::make_shared<ReturnVal>(m_latestReturnedRow.get(), nullptr);
+      return ReturnVal(m_latestReturnedRow.get(), nullptr);
     }
   }
 
   /** It looks like no one will decrease the ref of this Py_None, so we don't
    * increment the ref count here */
-  return std::make_shared<ReturnVal>(Py_None, nullptr);
+  return ReturnVal(Py_None, nullptr);
 }
 
 void CArrowChunkIterator::createRowPyObject()

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/CArrowChunkIterator.hpp

@@ -37,7 +37,7 @@ class CArrowChunkIterator : public CArrowIterator
   /**
    * @return a python tuple object which contains all data in current row
    */
-  std::shared_ptr<ReturnVal> next() override;
+  ReturnVal next() override;
 
 protected:
   /**

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/CArrowIterator.cpp

@@ -10,6 +10,48 @@
 namespace sf
 {
 
+const char* const NANOARROW_TYPE_ENUM_STRING[] = {
+    "NANOARROW_TYPE_UNINITIALIZED",
+    "NANOARROW_TYPE_NA",
+    "NANOARROW_TYPE_BOOL",
+    "NANOARROW_TYPE_UINT8",
+    "NANOARROW_TYPE_INT8",
+    "NANOARROW_TYPE_UINT16",
+    "NANOARROW_TYPE_INT16",
+    "NANOARROW_TYPE_UINT32",
+    "NANOARROW_TYPE_INT32",
+    "NANOARROW_TYPE_UINT64",
+    "NANOARROW_TYPE_INT64",
+    "NANOARROW_TYPE_HALF_FLOAT",
+    "NANOARROW_TYPE_FLOAT",
+    "NANOARROW_TYPE_DOUBLE",
+    "NANOARROW_TYPE_STRING",
+    "NANOARROW_TYPE_BINARY",
+    "NANOARROW_TYPE_FIXED_SIZE_BINARY",
+    "NANOARROW_TYPE_DATE32",
+    "NANOARROW_TYPE_DATE64",
+    "NANOARROW_TYPE_TIMESTAMP",
+    "NANOARROW_TYPE_TIME32",
+    "NANOARROW_TYPE_TIME64",
+    "NANOARROW_TYPE_INTERVAL_MONTHS",
+    "NANOARROW_TYPE_INTERVAL_DAY_TIME",
+    "NANOARROW_TYPE_DECIMAL128",
+    "NANOARROW_TYPE_DECIMAL256",
+    "NANOARROW_TYPE_LIST",
+    "NANOARROW_TYPE_STRUCT",
+    "NANOARROW_TYPE_SPARSE_UNION",
+    "NANOARROW_TYPE_DENSE_UNION",
+    "NANOARROW_TYPE_DICTIONARY",
+    "NANOARROW_TYPE_MAP",
+    "NANOARROW_TYPE_EXTENSION",
+    "NANOARROW_TYPE_FIXED_SIZE_LIST",
+    "NANOARROW_TYPE_DURATION",
+    "NANOARROW_TYPE_LARGE_STRING",
+    "NANOARROW_TYPE_LARGE_BINARY",
+    "NANOARROW_TYPE_LARGE_LIST",
+    "NANOARROW_TYPE_INTERVAL_MONTH_DAY_NANO"
+};
+
 Logger* CArrowIterator::logger = new Logger("snowflake.connector.CArrowIterator");
 
 CArrowIterator::CArrowIterator(char* arrow_bytes, int64_t arrow_bytes_size)
@@ -53,10 +95,10 @@ CArrowIterator::CArrowIterator(char* arrow_bytes, int64_t arrow_bytes_size)
   logger->debug(__FILE__, __func__, __LINE__, "Arrow BatchSize: %d", m_ipcArrowArrayVec.size());
 }
 
-std::shared_ptr<ReturnVal> CArrowIterator::checkInitializationStatus()
+ReturnVal CArrowIterator::checkInitializationStatus()
 {
     SF_CHECK_PYTHON_ERR()
-    return std::make_shared<ReturnVal>(nullptr, nullptr);
+    return ReturnVal(nullptr, nullptr);
 }
 
 }

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/CArrowIterator.hpp

@@ -13,49 +13,6 @@
 #include <string>
 #include <vector>
 
-
-static const char* NANOARROW_TYPE_ENUM_STRING[] = {
-    "NANOARROW_TYPE_UNINITIALIZED",
-    "NANOARROW_TYPE_NA",
-    "NANOARROW_TYPE_BOOL",
-    "NANOARROW_TYPE_UINT8",
-    "NANOARROW_TYPE_INT8",
-    "NANOARROW_TYPE_UINT16",
-    "NANOARROW_TYPE_INT16",
-    "NANOARROW_TYPE_UINT32",
-    "NANOARROW_TYPE_INT32",
-    "NANOARROW_TYPE_UINT64",
-    "NANOARROW_TYPE_INT64",
-    "NANOARROW_TYPE_HALF_FLOAT",
-    "NANOARROW_TYPE_FLOAT",
-    "NANOARROW_TYPE_DOUBLE",
-    "NANOARROW_TYPE_STRING",
-    "NANOARROW_TYPE_BINARY",
-    "NANOARROW_TYPE_FIXED_SIZE_BINARY",
-    "NANOARROW_TYPE_DATE32",
-    "NANOARROW_TYPE_DATE64",
-    "NANOARROW_TYPE_TIMESTAMP",
-    "NANOARROW_TYPE_TIME32",
-    "NANOARROW_TYPE_TIME64",
-    "NANOARROW_TYPE_INTERVAL_MONTHS",
-    "NANOARROW_TYPE_INTERVAL_DAY_TIME",
-    "NANOARROW_TYPE_DECIMAL128",
-    "NANOARROW_TYPE_DECIMAL256",
-    "NANOARROW_TYPE_LIST",
-    "NANOARROW_TYPE_STRUCT",
-    "NANOARROW_TYPE_SPARSE_UNION",
-    "NANOARROW_TYPE_DENSE_UNION",
-    "NANOARROW_TYPE_DICTIONARY",
-    "NANOARROW_TYPE_MAP",
-    "NANOARROW_TYPE_EXTENSION",
-    "NANOARROW_TYPE_FIXED_SIZE_LIST",
-    "NANOARROW_TYPE_DURATION",
-    "NANOARROW_TYPE_LARGE_STRING",
-    "NANOARROW_TYPE_LARGE_BINARY",
-    "NANOARROW_TYPE_LARGE_LIST",
-    "NANOARROW_TYPE_INTERVAL_MONTH_DAY_NANO"
-};
-
 #define SF_CHECK_ARROW_RC(arrow_status, format_string, ...) \
   if (arrow_status != NANOARROW_OK) \
   { \
@@ -96,19 +53,29 @@ static const char* NANOARROW_TYPE_ENUM_STRING[] = {
     Py_XDECREF(type);\
     Py_XDECREF(traceback);\
 \
-    return std::make_shared<ReturnVal>(nullptr, m_currentPyException.get());\
+    return ReturnVal(nullptr, m_currentPyException.get());\
   }
 
 namespace sf
 {
 
+extern const char* const NANOARROW_TYPE_ENUM_STRING[];
+
 /**
  * A simple struct to contain return data back cython.
  * PyObject would be nullptr if failed and cause string will be populated
+ *
+ * Note that `ReturnVal` does not own these pointers, so they should
+ * not be decref'ed by the receiver.
  */
 class ReturnVal
 {
 public:
+  ReturnVal() :
+    successObj(nullptr), exception(nullptr)
+  {
+  }
+
   ReturnVal(PyObject * obj, PyObject *except) :
     successObj(obj), exception(except)
   {
@@ -133,12 +100,12 @@ class CArrowIterator
   /**
    * @return a python object which might be current row or an Arrow Table
    */
-  virtual std::shared_ptr<ReturnVal> next() = 0;
+  virtual ReturnVal next() = 0;
   virtual std::vector<uintptr_t> getArrowArrayPtrs() { return {}; };
   virtual std::vector<uintptr_t> getArrowSchemaPtrs() { return {}; };
 
   /** check whether initialization succeeded or encountered error */
-  std::shared_ptr<ReturnVal> checkInitializationStatus();
+  ReturnVal checkInitializationStatus();
 
 protected:
   static Logger* logger;

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/CArrowTableIterator.cpp

@@ -6,7 +6,6 @@
 #include "SnowflakeType.hpp"
 #include "Python/Common.hpp"
 #include "Util/time.hpp"
-#include <memory>
 #include <string>
 #include <cstring>
 #include <vector>
@@ -198,16 +197,16 @@ m_convert_number_to_decimal(number_to_decimal)
   PyArg_Parse(tz.get(), "s", &m_timezone);
 }
 
-std::shared_ptr<ReturnVal> CArrowTableIterator::next()
+ReturnVal CArrowTableIterator::next()
 {
   bool firstDone = this->convertRecordBatchesToTable_nanoarrow();
   if (firstDone && !m_ipcArrowArrayVec.empty())
   {
-    return std::make_shared<ReturnVal>(Py_True, nullptr);
+    return ReturnVal(Py_True, nullptr);
   }
   else
   {
-    return std::make_shared<ReturnVal>(Py_None, nullptr);
+    return ReturnVal(Py_None, nullptr);
   }
 }
 
@@ -305,10 +304,10 @@ void CArrowTableIterator::convertScaledFixedNumberColumnToDecimalColumn_nanoarro
         SF_CHECK_ARROW_RC(returnCode, "[Snowflake Exception] error appending null to arrow array, error code: %d", returnCode);
     } else {
         auto originalVal = ArrowArrayViewGetIntUnsafe(columnArray, rowIdx);
-        std::shared_ptr<ArrowDecimal> arrowDecimal = std::make_shared<ArrowDecimal>();
-        ArrowDecimalInit(arrowDecimal.get(), 128, 38, scale);
-        ArrowDecimalSetInt(arrowDecimal.get(), originalVal);
-        returnCode = ArrowArrayAppendDecimal(newArray, arrowDecimal.get());
+        ArrowDecimal arrowDecimal;
+        ArrowDecimalInit(&arrowDecimal, 128, 38, scale);
+        ArrowDecimalSetInt(&arrowDecimal, originalVal);
+        returnCode = ArrowArrayAppendDecimal(newArray, &arrowDecimal);
         SF_CHECK_ARROW_RC(returnCode, "[Snowflake Exception] error appending decimal to arrow array, error code: %d", returnCode);
     }
   }

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/CArrowTableIterator.hpp

@@ -42,7 +42,7 @@ class CArrowTableIterator : public CArrowIterator
   /**
    * @return an arrow table containing all data in all record batches
    */
-  std::shared_ptr<ReturnVal> next() override;
+  ReturnVal next() override;
   std::vector<uintptr_t> getArrowArrayPtrs() override;
   std::vector<uintptr_t> getArrowSchemaPtrs() override;
 

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/Python/Common.hpp

@@ -41,7 +41,7 @@ class UniqueRef
   {
   }
 
-  explicit UniqueRef(UniqueRef&& other) : UniqueRef(other.release())
+  UniqueRef(UniqueRef&& other) : UniqueRef(other.release())
   {
   }
 
@@ -56,10 +56,19 @@ class UniqueRef
     reset();
   }
 
+  // Make this `UniqueRef` take ownership of the reference
+  // `pyObj`, if it is not null. If this `UniqueRef` previously
+  // owned a reference, it is released (via decrementing the
+  // refcount).
+  //
+  // This provides a similar guarantee as `Py_SETREF()` or
+  // `Py_CLEAR()`, by setting the new reference before
+  // decrementing the old reference.
   void reset(PyObject* pyObj = nullptr)
   {
-    Py_XDECREF(m_pyObj);
+    PyObject *toDelete = m_pyObj;
     m_pyObj = pyObj;
+    Py_XDECREF(toDelete);
   }
 
   PyObject* release() noexcept

src/snowflake/connector/nanoarrow_cpp/ArrowIterator/nanoarrow_arrow_iterator.pyx

@@ -9,7 +9,6 @@
 from cpython.ref cimport PyObject
 from cython.operator cimport dereference
 from libc.stdint cimport int64_t, uint8_t, uintptr_t
-from libcpp.memory cimport shared_ptr
 from libcpp.vector cimport vector
 
 INSTALLED_PYARROW = False
@@ -38,8 +37,8 @@ cdef extern from "CArrowIterator.hpp" namespace "sf":
         PyObject * exception;
 
     cdef cppclass CArrowIterator:
-        shared_ptr[ReturnVal] next() except +;
-        shared_ptr[ReturnVal] checkInitializationStatus() except +;
+        ReturnVal next() except +;
+        ReturnVal checkInitializationStatus() except +;
         vector[uintptr_t] getArrowArrayPtrs();
         vector[uintptr_t] getArrowSchemaPtrs();
 
@@ -87,7 +86,6 @@ cdef class PyArrowIterator(EmptyPyArrowIterator):
     cdef object context
     cdef CArrowIterator* cIterator
     cdef str unit
-    cdef shared_ptr[ReturnVal] cret
     cdef object use_dict_result
     cdef object cursor
     cdef vector[uintptr_t] nanoarrow_Table
@@ -159,33 +157,33 @@ cdef class PyArrowRowIterator(PyArrowIterator):
             self.arrow_bytes_size,
             <PyObject *> self.use_numpy
             )
-        self.cret = self.cIterator.checkInitializationStatus()
-        if self.cret.get().exception:
+        cdef ReturnVal cret = self.cIterator.checkInitializationStatus()
+        if cret.exception:
             Error.errorhandler_wrapper(
                 self.cursor.connection if self.cursor is not None else None,
                 self.cursor,
                 OperationalError,
                 {
-                    'msg': f'Failed to open arrow stream: {str(<object>self.cret.get().exception)}',
+                    'msg': f'Failed to open arrow stream: {str(<object>cret.exception)}',
                     'errno': ER_FAILED_TO_READ_ARROW_STREAM
                 })
         snow_logger.debug(msg=f"Batches read: {self.cIterator.getArrowArrayPtrs().size()}", path_name=__file__, func_name="__cinit__")
 
     def __next__(self):
-        self.cret = self.cIterator.next()
-        if not self.cret.get().successObj:
+        cdef ReturnVal cret = self.cIterator.next()
+        if not cret.successObj:
             Error.errorhandler_wrapper(
                 self.cursor.connection if self.cursor is not None else None,
                 self.cursor,
                 InterfaceError,
                 {
-                    'msg': f'Failed to convert current row, cause: {<object>self.cret.get().exception}',
+                    'msg': f'Failed to convert current row, cause: {<object>cret.exception}',
                     'errno': ER_FAILED_TO_CONVERT_ROW_TO_PYTHON_TYPE
                 }
             )
             # it looks like this line can help us get into python and detect the global variable immediately
             # however, this log will not show up for unclear reason
-        ret = <object>self.cret.get().successObj
+        ret = <object>cret.successObj
 
         if ret is None:
             raise StopIteration
@@ -225,17 +223,17 @@ cdef class PyArrowTableIterator(PyArrowIterator):
             self.arrow_bytes_size,
             self.number_to_decimal,
         )
-        self.cret = self.cIterator.checkInitializationStatus()
-        if self.cret.get().exception:
+        cdef ReturnVal cret = self.cIterator.checkInitializationStatus()
+        if cret.exception:
             Error.errorhandler_wrapper(
                 self.cursor.connection if self.cursor is not None else None,
                 self.cursor,
                 OperationalError,
                 {
-                    'msg': f'Failed to open arrow stream: {str(<object>self.cret.get().exception)}',
+                    'msg': f'Failed to open arrow stream: {str(<object>cret.exception)}',
                     'errno': ER_FAILED_TO_READ_ARROW_STREAM
                 })
-        self.cret = self.cIterator.next()
+        cdef ReturnVal cret2 = self.cIterator.next()
         self.nanoarrow_Table = self.cIterator.getArrowArrayPtrs()
         self.nanoarrow_Schema = self.cIterator.getArrowSchemaPtrs()
         self.pyarrow_table = pyarrow.Table.from_batches(