Skip to content

feat(cuDF): Register not in cuDF function registry#16503

Open
pramodsatya wants to merge 1 commit intofacebookincubator:mainfrom
pramodsatya:cudf_not
Open

feat(cuDF): Register not in cuDF function registry#16503
pramodsatya wants to merge 1 commit intofacebookincubator:mainfrom
pramodsatya:cudf_not

Conversation

@pramodsatya
Copy link
Copy Markdown
Collaborator

@pramodsatya pramodsatya commented Feb 24, 2026

Motivation

Presto's planner and optimizer build not(...) call expressions during analysis and rewrite phases and resolve them using functions in the active function namespace. In native execution, the function namespace is populated from the sidecar's /v1/functions endpoint and when cuDF execution is enabled this list contains cuDF functions only. If not is missing in this list, function resolution fails on coordinator for simple queries with sidecar.

Change

Adding not to the cuDF built-in registry ensures it appears in the sidecar's function list and is resolvable during planning in cuDF-backed Presto C++ deployments with sidecar.

@netlify
Copy link
Copy Markdown

netlify bot commented Feb 24, 2026

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4aab01b
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/69ba387eea77c400085ed543

@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 Feb 24, 2026
@devavret
Copy link
Copy Markdown
Collaborator

devavret commented Mar 5, 2026

This is understandable. We should have a standalone Function implementation for everything that's supported in AST and JIT. I'm still curious where the AST failed to take over and NOT had to be implemented as a standalone function.

@devavret devavret added the cudf cudf related - GPU acceleration label Mar 12, 2026
@pramodsatya pramodsatya changed the title feat: Register not as a cudf function feat(cuDF): Register not in cuDF function registry Mar 18, 2026
@pramodsatya pramodsatya marked this pull request as ready for review March 18, 2026 05:30
@pramodsatya
Copy link
Copy Markdown
Collaborator Author

pramodsatya commented Mar 18, 2026

@devavret, the AST did not fail to take over. NOT is used internally by Presto during planning, so in cluster deployments with sidecar we need to ensure sidecar reports NOT as a supported cuDF function, otherwise simple queries fail with unsupported function error.

All registered cuDF scalar and aggregate functions will be consumed by Presto sidecar for better function management at coordinator; this will be followed by #16504 and Presto sidecar changes (prototyped in prestodb/presto#27164).

Could you help review this change?

@devavret
Copy link
Copy Markdown
Collaborator

we need to ensure sidecar reports NOT as a supported cuDF function, otherwise simple queries fail with unsupported function error.

This should've already happened. ExpressionEvaluator's canBeEvaluatedByCudf delegates this to AstExpression::canEvaluate and if that fails, it falls back to checking FunctionExpression::canEvaluate. In this case, NOT should've been caught as supported by the former. This PR is fine and I can approve it but I'd prefer to know if AstExpression::canEvaluate(NOT) returned false for you.

@pramodsatya
Copy link
Copy Markdown
Collaborator Author

pramodsatya commented Mar 18, 2026

When cuDF function management is enabled via sidecar, the queries will fail on the coordinator during planning before getting to canBeEvaluatedByCudf checks in AstExpression on the cuDF worker, elaborating a bit on the usecase:

The function validation via sidecar retrieves cuDF functions from the function registry, which is exposed to Presto in #16504. If NOT is missing from the function registry, the coordinator assumes during planning that NOT is an unsupported function and fails simple queries like SHOW SESSION. Registering NOT explicitly fixes this error.
The plan checker via sidecar uses canBeEvaluatedByCudf checks as you pointed out, and this is functioning correctly for NOT. These are two separate components of the sidecar plugin.

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. cudf cudf related - GPU acceleration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants