review fixes, but keep the leak

Author: myrrc

.github/workflows/ci.yml

@@ -384,10 +384,10 @@ jobs:
       fail-fast: false
       matrix:
         include:
+          # We don't run memory sanitizer as it provides many false positives
+          # for std
           - sanitizer: asan
-            sanitizer_flags: "-Zsanitizer=address -Zsanitize=leak"
-          - sanitizer: msan
-            sanitizer_flags: "-Zsanitizer=memory"
+            sanitizer_flags: "-Zsanitizer=address,leak"
           - sanitizer: tsan
             sanitizer_flags: "-Zsanitizer=thread"
     name: "Rust tests (${{ matrix.sanitizer }})"
@@ -419,15 +419,16 @@ jobs:
         run: |
           rustup toolchain install $NIGHTLY_TOOLCHAIN
           rustup component add --toolchain $NIGHTLY_TOOLCHAIN rust-src rustfmt clippy llvm-tools-preview
-          export RUSTFLAGS="${RUSTFLAGS} ${{ matrix.sanitizer_flags }}"
       - name: Build tests with sanitizer
         run: |
+          RUSTFLAGS="${RUSTFLAGS} ${{ matrix.sanitizer_flags }}" \
           cargo +$NIGHTLY_TOOLCHAIN build --locked --all-features \
             --target x86_64-unknown-linux-gnu -Zbuild-std \
             -p vortex-buffer -p vortex-fastlanes -p vortex-fsst -p vortex-alp -p vortex-array
 
       - name: Run tests with sanitizer
         run: |
+          RUSTFLAGS="${RUSTFLAGS} ${{ matrix.sanitizer_flags }}" \
           cargo +$NIGHTLY_TOOLCHAIN nextest run --locked --all-features \
             --target x86_64-unknown-linux-gnu --no-fail-fast -Zbuild-std \
             -p vortex-buffer -p vortex-fastlanes -p vortex-fsst -p vortex-alp -p vortex-array
@@ -438,6 +439,7 @@ jobs:
         # TODO(myrrc): remove --no-default-features once we make Mimalloc opt-in
       - name: Run vortex-ffi tests with sanitizer
         run: |
+          RUSTFLAGS="${RUSTFLAGS} ${{ matrix.sanitizer_flags }}" \
           cargo +$NIGHTLY_TOOLCHAIN test --locked --no-default-features \
             --target x86_64-unknown-linux-gnu --no-fail-fast -Zbuild-std \
             -p vortex-ffi -- --no-capture
@@ -450,7 +452,7 @@ jobs:
           # We don't run memory sanitizer as it's clang-only and provides many
           # false positives for Catch2
           - sanitizer: asan
-            sanitizer_flags: "-Zsanitizer=address -Zsanitize=leak"
+            sanitizer_flags: "-Zsanitizer=address,leak"
           - sanitizer: tsan
             sanitizer_flags: "-Zsanitizer=thread"
     name: "Rust/C++ FFI tests (${{ matrix.sanitizer }})"
@@ -484,15 +486,13 @@ jobs:
         run: |
           rustup toolchain install $NIGHTLY_TOOLCHAIN
           rustup component add --toolchain $NIGHTLY_TOOLCHAIN rust-src rustfmt clippy llvm-tools-preview
-
-          # Export flags here so that rustfilt won't be built with sanitizers
-          export RUSTFLAGS="-A warnings -Cunsafe-allow-abi-mismatch=sanitizer \
-                --cfg disable_loom --cfg vortex_nightly -C debuginfo=2 \
-                -C opt-level=0 -C strip=none -Zexternal-clangrt \
-                ${{ matrix.sanitizer_flags }}"
       - name: Build FFI library
         run: |
           # TODO(myrrc): remove --no-default-features
+          RUSTFLAGS="-A warnings -Cunsafe-allow-abi-mismatch=sanitizer \
+                --cfg disable_loom --cfg vortex_nightly -C debuginfo=2 \
+                -C opt-level=0 -C strip=none -Zexternal-clangrt \
+                ${{ matrix.sanitizer_flags }}" \
           cargo +$NIGHTLY_TOOLCHAIN build --locked --no-default-features \
             --target x86_64-unknown-linux-gnu -Zbuild-std \
             -p vortex-ffi

vortex-ffi/cinclude/vortex.h

@@ -165,14 +165,14 @@ typedef enum {
      */
     VX_OPERATOR_SUB = 9,
     /**
-     * Multiple two numbers
+     * Multiply two numbers
      */
     VX_OPERATOR_MUL = 10,
     /**
      * Divide the left side by the right side
      */
     VX_OPERATOR_DIV = 11,
-} vx_operator;
+} vx_binary_operator;
 
 /**
  * Log levels for the Vortex library.
@@ -555,7 +555,9 @@ const vx_string *vx_array_get_utf8(const vx_array *array, uint32_t index);
 const vx_binary *vx_array_get_binary(const vx_array *array, uint32_t index);
 
 /**
- * Apply the expression to the array, producing a new array in constant time
+ * Apply the expression to the array, wrapping it with a ScalarFnArray.
+ * This operation takes constant time as it doesn't execute the underlying
+ * array. Executing the underlying array still takes O(n) time.
  */
 const vx_array *vx_array_apply(const vx_array *array, const vx_expression *expression, vx_error **error);
 
@@ -758,86 +760,91 @@ void vx_error_free(vx_error *ptr);
  */
 const vx_string *vx_error_get_message(const vx_error *error);
 
-/**
- * Clone a borrowed [`vx_expression`], returning an owned [`vx_expression`].
- *
- *
- * Must be released with [`vx_expression_free`].
- */
-const vx_expression *vx_expression_clone(const vx_expression *ptr);
-
 /**
  * Free an owned [`vx_expression`] object.
  */
-void vx_expression_free(const vx_expression *ptr);
+void vx_expression_free(vx_expression *ptr);
 
 /**
- * Create a root expression
+ * Create a root expression. A root expression, applied to an array in
+ * vx_array_apply, takes the array itself as opposed to functions like
+ * vx_expression_column or vx_expression_select which take the array's parts.
+ *
+ * Example:
+ *
+ * const vx_array* array = ...;
+ * vx_expression* root = vx_expression_root();
+ * const vx_error* error = NULL;
+ * vx_array* applied_array = vx_array_apply(array, root, &error);
+ * // array and applied_array are identical
+ * vx_array_free(applied_array);
+ * vx_expression_free(root);
+ * vx_array_free(array);
+ *
  */
-const vx_expression *vx_expression_root(void);
+vx_expression *vx_expression_root(void);
 
 /**
  * Create an expression that accesses a field from the root array.
+ * Errors in vx_array_apply if the root array doesn't have a specified field.
  *
  * Equivalent to get_item(name, root()).
  */
-const vx_expression *vx_expression_column(const char *name);
+vx_expression *vx_expression_column(const char *name);
 
 /**
  * Create an expression that selects (includes) specific fields from a child
- * expression. Child expression must have a DTYPE_STRUCT dtype.
+ * expression. Child expression must have a DTYPE_STRUCT dtype. Errors in
+ * vx_array_apply if the child expression doesn't have a specified field.
  *
  * Example:
  *
- * const vx_expression* root = vx_expression_root();
+ * vx_expression* root = vx_expression_root();
  * const char* names[] = {"name", "age"};
- * const vx_expression* select = vx_expression_select(names, 2, root);
+ * vx_expression* select = vx_expression_select(names, 2, root);
  * vx_expression_free(select);
  * vx_expression_free(root);
  *
  */
-const vx_expression *vx_expression_select(const char *const *names, size_t len, const vx_expression *child);
+vx_expression *vx_expression_select(const char *const *names, size_t len, const vx_expression *child);
 
 /**
  * Create an AND expression for multiple child expressions.
- * If there are no input expressions, returns NULL.
+ * If there are no input expressions, returns a logical true expression
  */
-const vx_expression *vx_expression_and(const vx_expression *const *expressions, size_t len);
+vx_expression *vx_expression_and(const vx_expression *const *expressions, size_t len);
 
 /**
  * Create an OR disjunction expression for multiple child expressions.
  * If there are no input expressions, returns NULL.
  */
-const vx_expression *vx_expression_or(const vx_expression *const *expressions, size_t len);
+vx_expression *vx_expression_or(const vx_expression *const *expressions, size_t len);
 
 /**
  * Create a binary expression for two expressions of form lhs OP rhs.
  * If either input is NULL, returns NULL.
  *
- * Example:
+ * Example for a binary sum:
  *
- * const vx_expression* age = vx_expression_column("age");
- * const vx_expression* height = vx_expression_column("height");
- * const vx_expression* sum = vx_expression_binary(VX_OPERATOR_SUM, age, height);
+ * vx_expression* age = vx_expression_column("age");
+ * vx_expression* height = vx_expression_column("height");
+ * vx_expression* sum = vx_expression_binary(VX_OPERATOR_ADD, age, height);
  * vx_expression_free(sum);
  * vx_expression_free(height);
  * vx_expression_free(age);
  *
+ * Example for a binary equality function:
+ *
+ * vx_expression* vx_expression_eq(
+ *     const vx_expression* lhs,
+ *     const vx_expression* rhs
+ * ) {
+ *     return vx_expression_binary(VX_OPERATOR_EQ, lhs, rhs);
+ * }
+ *
  */
-const vx_expression *
-vx_expression_binary(vx_operator operator_, const vx_expression *lhs, const vx_expression *rhs);
-
-const vx_expression *vx_expression_eq(const vx_expression *lhs, const vx_expression *rhs);
-
-const vx_expression *vx_expression_not_eq(const vx_expression *lhs, const vx_expression *rhs);
-
-const vx_expression *vx_expression_lt(const vx_expression *lhs, const vx_expression *rhs);
-
-const vx_expression *vx_expression_lt_eq(const vx_expression *lhs, const vx_expression *rhs);
-
-const vx_expression *vx_expression_gt(const vx_expression *lhs, const vx_expression *rhs);
-
-const vx_expression *vx_expression_gt_eq(const vx_expression *lhs, const vx_expression *rhs);
+vx_expression *
+vx_expression_binary(vx_binary_operator operator_, const vx_expression *lhs, const vx_expression *rhs);
 
 /**
  * Create a logical NOT of the child expression.
@@ -851,23 +858,24 @@ const vx_expression *vx_expression_not(const vx_expression *child);
  *
  * Returns a boolean array indicating which positions contain null values.
  */
-const vx_expression *vx_expression_is_null(const vx_expression *child);
+vx_expression *vx_expression_is_null(const vx_expression *child);
 
 /**
  * Create an expression that extracts a named field from a struct expression.
  * Child expression must have a DTYPE_STRUCT dtype.
+ * Errors in vx_array_apply if the root array doesn't have a specified field.
  *
  * Accesses the specified field from the result of the child expression.
  * Equivalent to select(&item, 1, child).
  */
-const vx_expression *vx_expression_get_item(const char *item, const vx_expression *child);
+vx_expression *vx_expression_get_item(const char *item, const vx_expression *child);
 
 /**
  * Create an expression that checks if a value is contained in a list.
  *
  * Returns a boolean array indicating whether the value appears in each list.
  */
-const vx_expression *vx_expression_list_contains(const vx_expression *list, const vx_expression *value);
+vx_expression *vx_expression_list_contains(const vx_expression *list, const vx_expression *value);
 
 /**
  * Clone a borrowed [`vx_file`], returning an owned [`vx_file`].

vortex-ffi/src/array.rs

@@ -188,7 +188,9 @@ pub unsafe extern "C-unwind" fn vx_array_get_binary(
     }
 }
 
-/// Apply the expression to the array, producing a new array in constant time
+/// Apply the expression to the array, wrapping it with a ScalarFnArray.
+/// This operation takes constant time as it doesn't execute the underlying
+/// array. Executing the underlying array still takes O(n) time.
 #[unsafe(no_mangle)]
 pub unsafe extern "C" fn vx_array_apply(
     array: *const vx_array,
@@ -473,7 +475,7 @@ mod tests {
             // Test with Vortex Rust-side expressions here, test C API for
             // expressions in src/expressions.rs
             let expression = eq(root(), lit(3i32));
-            let expression = vx_expression::new(Arc::new(expression));
+            let expression = vx_expression::new(Box::new(expression));
 
             let res = vx_array_apply(ptr::null(), expression, &raw mut error);
             assert!(res.is_null());