Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cpp/src/transform/transform.cu
Original file line number Diff line number Diff line change
Expand Up @@ -1521,8 +1521,12 @@ transform_program::transform_program(
impl_->ast_input_types_.push_back(std::visit([](auto& view) { return view.type(); }, input));
impl_->ast_input_nullable_.push_back(
std::visit([](auto& view) { return view.nullable(); }, input));
if (auto const* scalar = std::get_if<scalar_column_view>(&input)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised by Claude verbatim, so take it with a grain of salt.


Dropping impl_->ast_scalar_columns_ = std::move(args.scalar_columns) leaves transform_args::scalar_columns orphaned: it's still populated at cpp/src/jit/row_ir.cpp:1030-1034, but no production code reads it anymore — the only remaining reader is cpp/tests/jit/row_ir.cpp:359-360. A populated-but-never-read ownership vector is a trap for the next reader, who will reasonably assume the program still owns those columns.

It also costs an extra allocation per cudf::scalar literal: the converter materializes a one-row column, we copy it here, and the original is freed when args goes out of scope. main moved it instead.

Would it make sense to normalize ownership in the converter instead — have the loop at cpp/src/jit/row_ir.cpp:1030-1034 also push a copy for the non-owning scalar_column_view alternative? Then this constructor keeps its single std::move, scalar_columns stops being dead, and the "one entry per nullopt in input_column_indices" invariant lives in one place rather than being re-derived here. That means touching row_ir.*, which is outside the current diff, so could be done in a follow-up.

If you'd rather keep the copy here, can we at least assert the correspondence — e.g. CUDF_EXPECTS(scalar_index == impl_->ast_scalar_columns_.size(), …) after the loop in run()? The alignment currently rests on several unstated facts in row_ir.cpp. In particular, add_input(column_view const&) produces a column_input with a nullopt column_index, which would desynchronize the two. Nothing in cpp/src calls it today — the only callers are the direct instance_context tests in cpp/tests/jit/row_ir.cpp, which never go through ast_converter::compute_table — but nothing stops a future caller either.

// The program must outlive non-owning scalar-column literals in the source AST.
impl_->ast_scalar_columns_.push_back(
std::make_unique<column>(scalar->as_column_view(), stream, mr));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
impl_->ast_scalar_columns_ = std::move(args.scalar_columns);
impl_->ast_input_column_indices_ = std::move(args.input_column_indices);
impl_->ast_outputs_ = std::move(args.outputs);
}
Expand Down
46 changes: 46 additions & 0 deletions cpp/tests/ast/transform_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,52 @@ TEST_F(TransformProgramTest, ReusesAstWithCompatibleTable)
CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view(), verbosity);
}

TEST_F(TransformProgramTest, OwnsScalarColumnViewLiterals)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to also add a test that combines a literal built from cudf::scalar with one built from a scalar_column_view? This is the ordering issue that this PR fixes, and not a hypothetical — any operation that synthesizes a scalar (e.g., "rescale") would produce it.

{
std::unique_ptr<cudf::transform_program> program;
{
auto construction_input = column_wrapper<int32_t>{3, 20, 1, 50};
auto construction_table = cudf::table_view{{construction_input}};
auto literal_column = column_wrapper<int32_t>{2};
Comment thread
igorpeshansky marked this conversation as resolved.
auto column_ref = cudf::ast::column_reference{0};
auto literal = cudf::ast::literal{cudf::scalar_column_view{literal_column}};
auto expression = cudf::ast::operation{cudf::ast::ast_operator::ADD, column_ref, literal};
std::reference_wrapper<cudf::ast::expression const> expressions[] = {expression};

program = std::make_unique<cudf::transform_program>(construction_table, expressions);
}
Comment on lines +115 to +126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Optional] An IIFE lets you do it in one assignment:

Suggested change
std::unique_ptr<cudf::transform_program> program;
{
auto construction_input = column_wrapper<int32_t>{3, 20, 1, 50};
auto construction_table = cudf::table_view{{construction_input}};
auto literal_column = column_wrapper<int32_t>{2};
auto column_ref = cudf::ast::column_reference{0};
auto literal = cudf::ast::literal{cudf::scalar_column_view{literal_column}};
auto expression = cudf::ast::operation{cudf::ast::ast_operator::ADD, column_ref, literal};
std::reference_wrapper<cudf::ast::expression const> expressions[] = {expression};
program = std::make_unique<cudf::transform_program>(construction_table, expressions);
}
std::unique_ptr<cudf::transform_program> program = []() {
auto construction_input = column_wrapper<int32_t>{3, 20, 1, 50};
auto construction_table = cudf::table_view{{construction_input}};
auto literal_column = column_wrapper<int32_t>{2};
auto column_ref = cudf::ast::column_reference{0};
auto literal = cudf::ast::literal{cudf::scalar_column_view{literal_column}};
auto expression = cudf::ast::operation{cudf::ast::ast_operator::ADD, column_ref, literal};
std::reference_wrapper<cudf::ast::expression const> expressions[] = {expression};
return std::make_unique<cudf::transform_program>(construction_table, expressions);
}();

Also in OwnsStringScalarColumnViewLiterals


auto input = column_wrapper<int32_t>{10, 20, 30};
auto table = cudf::table_view{{input}};
auto expected = column_wrapper<int32_t>{12, 22, 32};
auto result = std::move(program->run(table)->release().front());

CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view(), verbosity);
}

TEST_F(TransformProgramTest, OwnsStringScalarColumnViewLiterals)
{
std::unique_ptr<cudf::transform_program> program;
{
auto construction_input = cudf::test::strings_column_wrapper{"a", "ccc"};
auto construction_table = cudf::table_view{{construction_input}};
auto literal_column = cudf::test::strings_column_wrapper{"ccc"};
auto column_ref = cudf::ast::column_reference{0};
auto literal = cudf::ast::literal{cudf::scalar_column_view{literal_column}};
auto expression = cudf::ast::operation{cudf::ast::ast_operator::LESS, column_ref, literal};
std::reference_wrapper<cudf::ast::expression const> expressions[] = {expression};

program = std::make_unique<cudf::transform_program>(construction_table, expressions);
}

auto input = cudf::test::strings_column_wrapper{"a", "ccc", "dddd"};
auto table = cudf::table_view{{input}};
auto expected = column_wrapper<bool>{true, false, false};
auto result = std::move(program->run(table)->release().front());

CUDF_TEST_EXPECT_COLUMNS_EQUAL(expected, result->view(), verbosity);
}

TEST_F(TransformProgramTest, RejectsIncompatibleTable)
{
auto construction_input = column_wrapper<int32_t>{3, 20, 1, 50};
Expand Down
9 changes: 8 additions & 1 deletion java/src/main/java/ai/rapids/cudf/MemoryCleaner.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

package ai.rapids.cudf;

import ai.rapids.cudf.ast.AstJitProgram;
import ai.rapids.cudf.ast.CompiledExpression;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -367,7 +368,13 @@ static void register(CuFileHandle handle, Cleaner cleaner) {
}

public static void register(CompiledExpression expr, Cleaner cleaner) {
all.put(cleaner.id, new CleanerWeakReference(expr, cleaner, collected, false));
// Both compilation modes retain device-backed literal values.
all.put(cleaner.id, new CleanerWeakReference(expr, cleaner, collected, true));
}

public static void register(AstJitProgram program, Cleaner cleaner) {
// AST programs retain copied literal columns across evaluations.
all.put(cleaner.id, new CleanerWeakReference(program, cleaner, collected, true));
}

static void register(HybridScanReader reader, Cleaner cleaner) {
Expand Down
27 changes: 26 additions & 1 deletion java/src/main/java/ai/rapids/cudf/ast/AstExpression.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,39 @@ void serialize(ByteBuffer bb) {
}
}

/**
* Compile this expression for execution with the process-level backend selection.
*
* @return expression compatible with default AST consumers
Comment thread
igorpeshansky marked this conversation as resolved.
* @throws IllegalArgumentException if a root literal requires JIT compilation
* @throws ai.rapids.cudf.CudfException if compilation fails
*/
public CompiledExpression compile() {
return compile(CompiledExpression.CompilationMode.DEFAULT);
}

/**
* Compile this expression for explicit execution with the libcudf JIT backend.
* The returned expression cannot be used as a join or scan predicate.
*
* @return expression specialized for JIT execution
* @throws ai.rapids.cudf.CudfException if compilation fails
*/
public CompiledExpression compileJit() {
return compile(CompiledExpression.CompilationMode.JIT);
}

private CompiledExpression compile(CompiledExpression.CompilationMode mode) {
validateCompilationMode(mode);
int size = getSerializedSize();
ByteBuffer bb = ByteBuffer.allocate(size);
bb.order(ByteOrder.nativeOrder());
serialize(bb);
return new CompiledExpression(bb.array());
return new CompiledExpression(bb.array(), mode);
}

void validateCompilationMode(CompiledExpression.CompilationMode mode) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(a) This needs a Javadoc.
(b) This method is root-only, isn't it? Can we enforce that somehow? We should at least document that this should never recurse into children… Maybe rename to validateRootCompilationMode?


/** Get the size in bytes of the serialized form of this node and all child nodes */
abstract int getSerializedSize();

Expand Down
159 changes: 159 additions & 0 deletions java/src/main/java/ai/rapids/cudf/ast/AstJitProgram.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package ai.rapids.cudf.ast;

import ai.rapids.cudf.MemoryCleaner;
import ai.rapids.cudf.NativeDepsLoader;
import ai.rapids.cudf.Table;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Objects;

/**
* A reusable AST JIT program specialized to an input schema.
* Construction lowers the expressions and retrieves their JIT kernel. Subsequent calls reuse that
* kernel with tables whose referenced columns have compatible types and nullability.
* Callers must ensure that {@link #close()} does not overlap with {@link #computeTable(Table)}.
*/
public final class AstJitProgram implements AutoCloseable {
static {
NativeDepsLoader.loadNativeDeps();
}

private static final Logger log = LoggerFactory.getLogger(AstJitProgram.class);

private static final class AstJitProgramCleaner extends MemoryCleaner.Cleaner {
private long nativeHandle;

AstJitProgramCleaner(long nativeHandle) {
this.nativeHandle = nativeHandle;
}

@Override
protected synchronized boolean cleanImpl(boolean logErrorIfNotClean) {
boolean alreadyClean = nativeHandle == 0;
if (alreadyClean) {
return false;
}
long origAddress = nativeHandle;
try {
destroy(nativeHandle);
} finally {
nativeHandle = 0;
}
if (logErrorIfNotClean) {
log.error("AN AST JIT PROGRAM WAS LEAKED (ID: {} {})", id,
Long.toHexString(origAddress));
}
return true;
}

@Override
public boolean isClean() {
return nativeHandle == 0;
}
}

private final AstJitProgramCleaner cleaner;
private boolean isClosed = false;

private AstJitProgram(long nativeHandle) {
AstJitProgramCleaner newCleaner = null;
try {
newCleaner = new AstJitProgramCleaner(nativeHandle);
cleaner = newCleaner;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea to assign it as the last line of the try block, and use newCleaner for register/addRef:

    AstJitProgramCleaner newCleaner = null;
    try {
      newCleaner = new AstJitProgramCleaner(nativeHandle);
      MemoryCleaner.register(this, newCleaner);
      newCleaner.addRef();
      cleaner = newCleaner;
    } catch (Throwable t) {

Also in the CompiledExpression constructor…

[Really optional] Sigh, now we have two classes with an identical complex constructor pattern. It's probably way beyond the scope of this PR (and in scope for #23939), but we could fold a lot of this in by reusing Java's try-with-resources, e.g., get suppressed exception behavior for free:

  static abstract class CleanerManager<T extends MemoryCleaner.Cleaner> implements AutoCloseable {
    private T cleaner = null;
    private boolean released = false;
    public final T acquire(T c) { cleaner = c; return c; }
    public final T release() { released = true; return cleaner; }
    @Override
    public final void close() {
      if (released) return;
      if (cleaner != null) { cleaner.clean(false); }
      else { fallbackClean(); }
    }
    protected abstract void fallbackClean();
  }

  private AstJitProgram(long nativeHandle) {
    try (CleanerManager<AstJitProgramCleaner> manager =
             new CleanerManager<AstJitProgramCleaner>() {
               @Override
               protected void fallbackClean() { destroy(nativeHandle); }
             }) {
      AstJitProgramCleaner newCleaner = manager.acquire(new AstJitProgramCleaner(nativeHandle));
      MemoryCleaner.register(this, newCleaner);
      newCleaner.addRef();
      cleaner = manager.release();
    }
  }

MemoryCleaner.register(this, cleaner);
cleaner.addRef();
} catch (Throwable t) {
try {
if (newCleaner == null) {
destroy(nativeHandle);
} else {
newCleaner.clean(false);
}
} catch (Throwable cleanupFailure) {
t.addSuppressed(cleanupFailure);
}
throw t;
}
}

/**
* Compile a reusable program from one or more JIT-compiled expressions.
* The schema table and expressions are inspected during construction but are not retained. The
* returned program owns any literal values required by later evaluations.
*
* @param schemaTable table whose referenced column schema is used to compile the program
* @param expressions non-empty JIT-compiled expressions in output order
* @return a reusable AST JIT program
* @throws NullPointerException if the table, expression array, or an expression is null
* @throws IllegalArgumentException if no expressions are provided or an expression was not
* produced by {@link AstExpression#compileJit()}
* @throws IllegalStateException if the table or an expression is closed
* @throws ai.rapids.cudf.CudfException if JIT compilation fails
*/
public static AstJitProgram compile(Table schemaTable, CompiledExpression... expressions) {
long tableHandle = Objects.requireNonNull(schemaTable, "schemaTable").getNativeView();
CompiledExpression.JitExpressionArgs expressionArgs =
CompiledExpression.getJitExpressionArgs(expressions);
Comment on lines +101 to +102

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to do the closed table check before processing all of the expressions? Otherwise it's a behavior change. Also in CompiledExpression.computeTableJit

if (tableHandle == 0) {
throw new IllegalStateException("Table is closed");
}

try {
return new AstJitProgram(create(expressionArgs.nativeHandles, tableHandle));
} finally {
CompiledExpression.reachabilityFence(schemaTable);
CompiledExpression.reachabilityFence(expressionArgs.expressionRefs);
}
}

/**
* Evaluate this program on a table with a compatible referenced-column schema.
* The row count and unreferenced columns may differ from the schema table used at compilation.
* Calling {@link #close()} while an evaluation is in progress is unsupported.
*
* @param table input table for expression evaluation
* @return table containing the program outputs in expression order
* @throws NullPointerException if the table is null
* @throws IllegalStateException if the program or table is closed
* @throws ai.rapids.cudf.CudfException if the referenced-column schema is incompatible or
* evaluation fails
*/
public Table computeTable(Table table) {
long tableHandle = Objects.requireNonNull(table, "table").getNativeView();
long programHandle = cleaner.nativeHandle;
if (programHandle == 0) {
throw new IllegalStateException("AST JIT program is closed");
}
if (tableHandle == 0) {
throw new IllegalStateException("Table is closed");
}

try {
return new Table(computeTableNative(programHandle, tableHandle));
} finally {
CompiledExpression.reachabilityFence(this);
CompiledExpression.reachabilityFence(table);
}
}

@Override
public synchronized void close() {
cleaner.delRef();
if (isClosed) {
cleaner.logRefCountDebug("double free " + this);
throw new IllegalStateException("Close called too many times " + this);
}
cleaner.clean(false);
isClosed = true;
Comment on lines +147 to +153

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a bunch of these identical close() methods around the codebase. Out of scope in this PR, but yet another thing #23939 could potentially address (e.g., by creating a common superclass for objects with a native handle, or a NativeHandleManager these could delegate to)…

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, let's keep this comment open for tracking.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're keeping this open, I'll also add that since cleaner.delRef() is called before the isClosed check, a double-close will decrement the refcount unconditionally before throwing. This seems to be a recurring pattern, so also something to look into as part of #23939.

}

private static native long create(long[] astHandles, long tableHandle);
private static native long[] computeTableNative(long programHandle, long tableHandle);
private static native void destroy(long handle);
}
Loading
Loading