refactor(cuDF): Separate cuDF query and system configs by pramodsatya · Pull Request #16535 · facebookincubator/velox

pramodsatya · 2026-02-25T21:53:39Z

Separates out cuDF configs into system and query level configs. cuDF query configs are moved to core/QueryConfig and accessed through queryCtx, avoiding the use of singleton instance and allowing for different configs to be propagated in different sessions via Presto C++.
Extracts CudfConfig into a dedicated velox_cudf_config library within a new common/ subdirectory to enable external engines to consume the config module independently of exec operators.

Key Changes

Maintain distinction between CudfSystemConfig and cuDF QueryConfigs
Maintain hyphenated naming convention for system configs in CudfSystemConfig, while retaining backwards compatibility for underscored config names
Simplify CudfConfig API: Replace specialized template methods with two generic getters (get<T>(), getOptional<T>()) with automatic type conversion
Remove public member variables; all configuration accessed through getter/setter methods
Decouple config from exec layer: exec operators now link against velox_cudf_config library

Benefits

Enables other engines to set cuDF QueryConfigs cleanly, such as via session properties in Presto, and without an exec operator dependency.

netlify · 2026-02-25T21:53:46Z

✅ Deploy Preview for meta-velox ready!

Name	Link
🔨 Latest commit	`1090507`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/69c411cb6121cc0008eae563
😎 Deploy Preview	https://deploy-preview-16535--meta-velox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

velox/experimental/cudf/common/CudfConfig.h

jinchengchenghh · 2026-02-26T17:23:36Z

velox/experimental/cudf/common/CudfConfig.h

-  /// Clients can disable here and enable it via the QueryConfig as well.
-  bool enabled{true};
+  /// Retrieve all config keys currently in the map.
+  std::vector<std::string> getConfigKeys() const;


When do you need the keys?

These velox config keys are mapped to Presto session properties in prestodb/presto#27204.

jinchengchenghh · 2026-02-26T17:24:20Z

velox/experimental/cudf/common/CudfConfig.h

-  std::string memoryResource{"async"};
+  /// Retrieve config value by key, fails if config does not exist.
+  template <typename T>
+  T get(const std::string& key) const {


remove since super class has this API

super class does not have the same API, could you please clarify?
I found super class API that also expects a default value to be provided, this overload simplifies get and can be used when we are certain the config exists (such as in testcases) and get will not return std::nullopt.

I think we should follow the existing config usage pattern in S3, Config.h, etc. to keep configs consistent.

velox/experimental/cudf/common/CudfConfig.h

jinchengchenghh · 2026-02-26T17:29:11Z

velox/experimental/cudf/exec/CudfHashJoin.cpp

@@ -398,7 +398,7 @@ CudfHashJoinProbe::CudfHashJoinProbe(
        "Join field {} not in probe or build input", outputType->children()[i]);
  }

-  if (CudfConfig::getInstance().debugEnabled) {
+  if (CudfConfig::getInstance().get<bool>(CudfConfig::kCudfDebugEnabled)) {


But I wonder if we should use as this, as the QueryConfig, it has per function for per config and return the specified data type

I feel this is cleaner and having a separate getter method for each config is a maintenance overhead. I further changed the config entries to be of type CudfConfigEntry for better config metadata maintenance.

Could you please take another look @jinchengchenghh and share your thoughts on this?
The metadata fields added to CudfConfigEntry are required for prestodb/presto#27204.

What do you think about that? @majetideepak

majetideepak · 2026-03-02T19:49:38Z

velox/common/config/Config.h

@@ -118,6 +119,12 @@ class ConfigBase : public IConfig {

  const std::unordered_map<std::string, std::string>& rawConfigs() const;

+  /// Get the TypePtr for a config key. Subclasses can override to provide type
+  /// info.
+  virtual velox::TypePtr getType(const std::string& key) const {


Is this used?

majetideepak · 2026-03-02T19:52:00Z

velox/experimental/cudf/common/CudfConfig.h

+  /// Represents a single configuration entry with its metadata.
+  struct CudfConfigEntry {
+    std::string name; // Config key (e.g., "cudf.enabled")
+    velox::TypePtr type; // Type of the configuration value


Where do we use this type field?

majetideepak · 2026-03-02T20:15:59Z

velox/experimental/cudf/common/CudfConfig.h

-  std::string memoryResource{"async"};
+  /// Retrieve config value by key, fails if config does not exist.
+  template <typename T>
+  T get(const std::string& key) const {


I think we should follow the existing config usage pattern in S3, Config.h, etc. to keep configs consistent.

majetideepak · 2026-03-02T22:03:56Z

velox/experimental/cudf/exec/CudfFilterProject.cpp

@@ -255,7 +256,8 @@ RowVectorPtr CudfFilterProject::getOutput() {
  stream.synchronize();
  auto const numColumns = outputTable->num_columns();
  auto const size = outputTable->num_rows();
-  if (CudfConfig::getInstance().debugEnabled) {
+  if (CudfConfig::getInstance().get<bool>(


This should be wrapped in a common function where we check if the session property exists and return that or the core config.

pramodsatya · 2026-03-18T04:30:34Z

Thanks for the feedback @majetideepak, addressed the review comments and updated the PR to separate out cuDF system and query level configs as discussed. The cuDF query configs are now passed via queryCtx and the system config names are modified to use hyphenated format, while retaining support for the legacy underscore formatted names as well.
Could you please take another look?

majetideepak · 2026-03-20T20:20:53Z

velox/experimental/cudf/exec/CudfTopN.cpp

      topNNode_(topNNode),
-      kBatchSize_(CudfConfig::getInstance().topNBatchSize),
+      kBatchSize_(
+          operatorCtx_->execCtx()->queryCtx()->cudfConfig().topNBatchSize()),


This should be

topNBatchSize(operatorCtx_->execCtx()->queryCtx());

Inside topNBatchSize(), we check if the queryCtx has the session key cudf_topn_batch_size" or use the system config.

Updated, thanks.

majetideepak · 2026-03-27T19:17:00Z

velox/experimental/cudf/common/CudfSystemConfig.h

+  ///
+  /// Example: "cudf.allow_cpu_fallback" and "cudf.allow-cpu-fallback" are
+  /// normalized to canonical internal key "cudf.allow-cpu-fallback".
+  void updateConfigs(std::unordered_map<std::string, std::string>&&);


majetideepak · 2026-03-27T19:24:32Z

velox/experimental/cudf/common/CudfSystemConfig.h

+  CudfSystemConfig();
+
+  /// Constructor that initializes config from a map of values.
+  explicit CudfSystemConfig(


Let's remove

majetideepak · 2026-03-27T19:25:10Z

velox/experimental/cudf/common/CudfSystemConfig.cpp

+  return instance;
+}
+
+void CudfSystemConfig::validateConfigs() {


remove this function and add the body to updateConfigs.

majetideepak · 2026-03-27T19:25:31Z

velox/experimental/cudf/common/CudfSystemConfig.cpp

+void CudfSystemConfig::updateConfigs(
+    std::unordered_map<std::string, std::string>&& config) {
+  for (auto& [key, value] : config) {
+    // TODO(ps): Revert support for legacy config names.


Add an example of legacy config.

Throw a warning for legacy config?

majetideepak · 2026-03-27T19:31:22Z

velox/experimental/cudf/expression/AstExpressionUtils.h

-    auto const& geLower = tree.push(Operation{Op::GREATER_EQUAL, value, lower});
-    auto const& leUpper = tree.push(Operation{Op::LESS_EQUAL, value, upper});
-    return tree.push(Operation{Op::NULL_LOGICAL_AND, geLower, leUpper});
+      VELOX_CHECK_EQ(len, 3);


extra padding?

majetideepak

few nits and cleanup pending.

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 25, 2026

This was referenced Feb 25, 2026

refactor(cuDF): Consolidate cuDF configurations into single config file #16528

Closed

feat(native): Add cuDF session property provider prestodb/presto#27204

Draft

pramodsatya force-pushed the cudf_cfg branch 4 times, most recently from ed7ddbb to c639cc5 Compare February 26, 2026 06:05

pramodsatya changed the title ~~feat(cuDF): Add getter setter methods to CudfConfig~~ feat(cuDF): Expose CudfConfig to external engines with getter setter methods Feb 26, 2026

pramodsatya requested a review from majetideepak February 26, 2026 06:06

pramodsatya marked this pull request as ready for review February 26, 2026 06:06

pramodsatya requested review from bdice, devavret, karthikeyann and mhaseeb123 as code owners February 26, 2026 06:06

jinchengchenghh reviewed Feb 26, 2026

View reviewed changes

velox/experimental/cudf/common/CudfConfig.h Outdated Show resolved Hide resolved

pramodsatya force-pushed the cudf_cfg branch from d2cf7b3 to d7489a6 Compare February 26, 2026 17:04

pramodsatya changed the title ~~feat(cuDF): Expose CudfConfig to external engines with getter setter methods~~ refactor(cuDF): Expose CudfConfig to external engines Feb 26, 2026

jinchengchenghh reviewed Feb 26, 2026

View reviewed changes

pramodsatya force-pushed the cudf_cfg branch from d7489a6 to 6af02d4 Compare February 27, 2026 03:50

majetideepak reviewed Mar 2, 2026

View reviewed changes

pramodsatya marked this pull request as draft March 3, 2026 15:57

pramodsatya force-pushed the cudf_cfg branch from 6af02d4 to 3ae30a4 Compare March 9, 2026 22:01

devavret added the cudf cudf related - GPU acceleration label Mar 12, 2026

pramodsatya force-pushed the cudf_cfg branch 2 times, most recently from 8277308 to d90088f Compare March 18, 2026 04:25

pramodsatya requested a review from majetideepak March 18, 2026 04:27

pramodsatya marked this pull request as ready for review March 18, 2026 04:27

pramodsatya force-pushed the cudf_cfg branch from d90088f to d3e2367 Compare March 18, 2026 04:43

pramodsatya force-pushed the cudf_cfg branch from a98e666 to 13a2c6d Compare March 18, 2026 05:51

pramodsatya changed the title ~~refactor(cuDF): Expose CudfConfig to external engines~~ refactor(cuDF): Separate cuDF query and system configs and access cuDF query configs via QueryCtx Mar 18, 2026

majetideepak reviewed Mar 20, 2026

View reviewed changes

refactor(cuDF): Separate cuDF query and system configs

1090507

pramodsatya force-pushed the cudf_cfg branch from 13a2c6d to 1090507 Compare March 25, 2026 16:48

pramodsatya changed the title ~~refactor(cuDF): Separate cuDF query and system configs and access cuDF query configs via QueryCtx~~ refactor(cuDF): Separate cuDF query and system configs Mar 25, 2026

majetideepak reviewed Mar 27, 2026

View reviewed changes

Conversation

pramodsatya commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes

Benefits

Uh oh!

netlify bot commented Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox ready!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pramodsatya commented Mar 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pramodsatya commented Feb 25, 2026 •

edited

Loading

netlify bot commented Feb 25, 2026 •

edited

Loading