Skip to content

feat(cudf): Add NotFunction, IsNullFunction, IsNotNullFunction#17314

Open
mattgara wants to merge 4 commits intofacebookincubator:mainfrom
mattgara:logical-null-functions
Open

feat(cudf): Add NotFunction, IsNullFunction, IsNotNullFunction#17314
mattgara wants to merge 4 commits intofacebookincubator:mainfrom
mattgara:logical-null-functions

Conversation

@mattgara
Copy link
Copy Markdown
Collaborator

  • Implements IS NULL, IS NOT NULL and NOT functions

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 23, 2026

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 54149e4
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69ea4ed252fe440008829f5c

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 23, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

Build Impact Analysis

Full build recommended. Files outside the dependency graph changed:

  • velox/experimental/cudf/expression/ExpressionEvaluator.cpp
  • velox/experimental/cudf/tests/CMakeLists.txt
  • velox/experimental/cudf/tests/LogicalFunctionsTest.cpp

These directories are not fully covered by the dependency graph. A full build is the safest option.

cmake --build _build/release

Slow path • Graph generated from PR branch

Copy link
Copy Markdown
Collaborator

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Small overlap with #16503 but I think @pramodsatya had a different purpose with that, trying to use registry in sidecar.

Comment on lines +1501 to +1505
registerCudfFunction(
"not",
[](const std::string&, const std::shared_ptr<velox::exec::Expr>& expr) {
return std::make_shared<NotFunction>(expr);
},
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of making a NotFunction, you can make a UnaryFunction and during registration, provide a factory like so:

Suggested change
registerCudfFunction(
"not",
[](const std::string&, const std::shared_ptr<velox::exec::Expr>& expr) {
return std::make_shared<NotFunction>(expr);
},
registerCudfFunction(
"not",
[](const std::string&, const std::shared_ptr<velox::exec::Expr>& expr) {
return std::make_shared<UnaryFunction>(expr, cudf::unary_operator::NOT);
},

@pramodsatya
Copy link
Copy Markdown
Collaborator

pramodsatya commented Apr 23, 2026

Small overlap with #16503 but I think @pramodsatya had a different purpose with that, trying to use registry in sidecar.

Noted @devavret, closing #16503 in favor of this PR (17314 subsumes 16503 as you pointed out).
I would still like to get #16504 in for Presto-cuDF functions integration with sidecar, could you please help review this PR?

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants