Skip to content

[bug] CudfNestedLoopJoin drops build byte limit and rejects two-sided timestamp filters #111

Description

@sperlingxx

Highlight (by human-in-loop)

  1. These are valid tests. Production code regressed; this is not a spark-flux-authored invalid CASE/eq dialect issue.
  2. buildByteLimitAndCleanup was added with bounded NLJ accounting in 73a13076e (feat(cudf): Harden MPP exchange and bounded operator execution). The cpp implementation disappeared in the Facebook-main sync #38; the header/config/test still described the limit.
  3. innerJoinWithTimestampFilter was added in 635816cba (Improve cuDF coverage for Spark queries). Two-sided gt(p_ts, b_ts) is still valid; AST/JIT gates TIMESTAMP so the compiler tried to precompute the whole predicate and threw sideIdx == -2.

Problem

Part of umbrella #100 (velox_cudf_nested_loop_join_test).

  • CudfNestedLoopJoinTest.buildByteLimitAndCleanup: expected an exception (CudfNestedLoopJoin build exceeds configured device-memory limit), but none was thrown (NestedLoopJoinTest.cpp:100).
  • CudfNestedLoopJoinTest.innerJoinWithTimestampFilter: VeloxRuntimeError INVALID_STATE: Expression spans both join sides and cannot be precomputed: gt(ROW["p_ts"],ROW["b_ts"]); sideIdx was -2 (AstExpressionUtils.h:523).

Root cause

Byte limit. CudfNestedLoopJoin.h still declares reserveBuildBytes / releaseBuildBytes / maxBuildBytes_, and cudf.nested_loop_join.max_build_bytes still exists. CudfNestedLoopJoin.cpp stopped calling them after #38 replaced the cpp with the upstream operator. The join retained the full build with no cap.

Separately, Values stays on CPU. Default CudfFromVelox coalescing (gpu_batch_size_rows=100000) merges the two tiny build pages into one GPU batch, so even a restored cap would not exercise “first batch retained, second trips the limit, close releases accounting.”

Timestamp filter. containsAstUnsupportedType keeps TIMESTAMP off the AST/JIT path. gt(p_ts, b_ts) then goes through compileSubExpression(), which cannot precompute an expression that spans both join sides. cuDF AST comparison operators still accept timestamp column_references, so the join filter can be compiled from native field refs.

Expected behavior

  • With cudf.nested_loop_join.max_build_bytes=16, the second build GPU batch throws the configured device-memory limit, and close releases already-accounted bytes.
  • Inner NLJ p_ts > b_ts matches DuckDB.

Acceptance criteria

  • Restore build-side byte accounting in CudfNestedLoopJoin.cpp (including close-time release and concat-failure release).
  • Compile two-sided timestamp comparisons as AST ops instead of cross-side precompute.
  • Pin velox.cudf.gpu_batch_size_rows=1 on buildByteLimitAndCleanup so the two Values pages stay distinct GPU batches.
  • Both cases pass, and the rest of velox_cudf_nested_loop_join_test still passes.

Validation

Reproduced and verified on HPDA dev 28798f623 (CUDA SM 86):

  • unfixed: no throw on the byte cap; timestamp filter throws sideIdx == -2;
  • fixed: both cases PASSED; full binary 58/58.

Prepared with assistance from Cursor.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingcudf

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions