feat: Allow custom S3 filesystem factory by ReemaAlzaid · Pull Request #17400 · facebookincubator/velox

ReemaAlzaid · 2026-05-04T15:29:00Z

What changes were proposed in this pull request?

This PR adds an optional filesystem factory parameter to registerS3FileSystem.

This is the Velox side change intended to unblock the corresponding Gluten custom S3 filesystem registration work in apache/gluten#12026.

With this change, callers can reuse Velox's existing S3 registration path while supplying a custom FileSystem implementation for s3:// paths. When no factory is provided, the current behavior is unchanged and Velox still constructs S3FileSystem.

This preserves the existing:

cache key behavior
filesystem caching
FileSink registration
finalizeS3FileSystem() behavior

This PR also adds a test that verifies getFileSystem() returns the custom subclass when a factory is registered

Why are the changes needed?

Velox currently hardcodes S3FileSystem construction inside S3 registration. That makes it difficult for integrations to customize S3 filesystem creation while still reusing Velox's registration and caching logic

This change exposes a small generic extension point without changing the default behavior.

Does this PR introduce any user facing change?

No.

How was this patch tested?

Added a unit test for custom S3 filesystem registration.
Verified RegisterS3FileSystem.cpp with a syntax compile using the existing compile database

netlify · 2026-05-04T15:29:07Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`19ebbe3`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/6a05d7ff17157a00072da608

github-actions · 2026-05-04T15:30:31Z

Build Impact Analysis

Selective Build Targets (building these covers all 14 affected)

cmake --build _build/release --target velox_query_replayer velox_read_benchmark velox_s3config_test velox_s3file_test velox_s3finalize_test velox_s3insert_test velox_s3metrics_test velox_s3multiendpoints_test velox_s3read_test velox_s3registration_test velox_tool_trace_test

Total affected: 14/575 targets

Affected targets (14)

Directly changed (9)

Target	Changed Files
`velox_query_trace_replayer_base`	RegisterS3FileSystem.h
`velox_read_benchmark_lib`	RegisterS3FileSystem.h
`velox_s3file_test`	RegisterS3FileSystem.h
`velox_s3fs`	RegisterS3FileSystem.cpp, RegisterS3FileSystem.h
`velox_s3insert_test`	RegisterS3FileSystem.h
`velox_s3metrics_test`	RegisterS3FileSystem.h
`velox_s3multiendpoints_test`	RegisterS3FileSystem.h
`velox_s3read_test`	RegisterS3FileSystem.h
`velox_s3registration_test`	RegisterS3FileSystem.h, S3FileSystemRegistrationTest.cpp

Transitively affected (5)

velox_query_replayer
velox_read_benchmark
velox_s3config_test
velox_s3finalize_test
velox_tool_trace_test

Fast path • Graph from main@59f88d56b95c2854723e46f26f37f9e341abb2c4

JkSelf

@ReemaAlzaid Thanks for your fix. LGTM except small comments.

Copilot

Pull request overview

Adds an extension point to Velox’s S3 filesystem registration to allow callers to supply a custom FileSystem factory for s3:// paths while preserving the existing caching and FileSink registration behavior.

Changes:

Extend registerS3FileSystem to accept an optional S3FileSystemFactory for custom S3 FileSystem construction.
Use the provided factory (when present) in the S3 filesystem generator; default behavior remains S3FileSystem.
Add a unit test verifying that getFileSystem() returns the custom S3 filesystem implementation when registered.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`velox/connectors/hive/storage_adapters/s3fs/tests/S3FileSystemRegistrationTest.cpp`	Registers S3 with a custom factory and adds a test asserting the returned filesystem type.
`velox/connectors/hive/storage_adapters/s3fs/RegisterS3FileSystem.h`	Introduces `S3FileSystemFactory` and extends the `registerS3FileSystem` API.
`velox/connectors/hive/storage_adapters/s3fs/RegisterS3FileSystem.cpp`	Stores the factory and uses it to create cached S3 filesystem instances.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

        initializeS3(logLevel, logLocation);
-        auto fs = std::make_shared<S3FileSystem>(bucketName, properties);
+        auto fs = fileSystemFactory
+            ? fileSystemFactory(bucketName, properties)
+            : std::make_shared<S3FileSystem>(bucketName, properties);
        instanceMap.insert({cacheKey, fs});
        return fs;


rui-mo

Thanks. And please also take a look at the Copilot comments.

rui-mo · 2026-05-13T01:50:20Z

+TEST_F(S3FileSystemRegistrationTest, customFileSystemFactory) {
+  auto hiveConfig = minioServer_->hiveConfig();
+  auto s3fs = filesystems::getFileSystem(kDummyPath, hiveConfig);
+  ASSERT_NE(std::dynamic_pointer_cast<CustomS3FileSystem>(s3fs), nullptr);


nit: could use VELOX_CHECK_NOT_NULL.

rui-mo · 2026-05-13T01:54:58Z

  return config->get<std::string>("hive.s3.endpoint").value();
 }

+class CustomS3FileSystem : public S3FileSystem {


For the customization, do we intend to override S3FileSystem::Impl? If so, we may need to make the relevant members protected.

S3FileSystem::Impl and impl_ are already protected in S3FileSystem.h, but the actual Impl definition lives in S3FileSystem.cpp, so it remains an internal implementation detail:
https://github.com/facebookincubator/velox/blob/main/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.h#L90-L92
https://github.com/facebookincubator/velox/blob/main/velox/connectors/hive/storage_adapters/s3fs/S3FileSystem.cpp#L230

The factory hook is intended to customize the FileSystem/S3FileSystem instance, not override Impl.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

github-actions · 2026-05-14T12:47:22Z

CI Failure Analysis

Auto-generated by the CI Failure Analysis workflow. This comment is updated in place each time CI fails on a new commit, so it always reflects the latest run — re-pushing or re-running CI will refresh the analysis below. Last updated 2026-05-14 14:33:06 UTC from workflow run 25864859103.

⚪ Aggregation Fuzzer with Presto as source of truth — FUZZER Failure View logs

All 4 instances crashed (seeds: 119425723, 680258782, 631281975, 995490894)

Error: Velox and reference DB results don't match at AggregationFuzzer.cpp:1066

Instance 1 (seed=119425723): stddev_samp with distinct — results mismatch with Presto
  Plan: Aggregation SINGLE [g0, g1] a0 := stddev_samp(ROW["c0"]) distinct
  File: velox/exec/fuzzer/AggregationFuzzer.cpp:1066

Instance 2 (seed=680258782): covar_samp with distinct — results mismatch with Presto
  Plan: Aggregation SINGLE [g0, g1, g2] a0 := covar_samp(ROW["c0"],ROW["c1"]) distinct

Instance 3 (seed=631281975): group-by only aggregation — results mismatch with Presto
  Plan: Aggregation SINGLE [g0] — MAP<TIMESTAMP WITH TIME ZONE,TIMESTAMP>

Instance 4 (seed=995490894): group-by only aggregation — results mismatch with Presto
  Plan: Aggregation SINGLE [g0] — ROW<...,TIMESTAMP WITH TIME ZONE,...,IPADDRESS>

⚪ Expression Fuzzer with Presto SOT — FUZZER Failure View logs

All 4 instances crashed (seeds: 564466436, 730604412, 630078507, 179380940)

Error: Velox and reference DB results don't match at ExpressionVerifier.cpp:475

Instance 1 (seed=564466436): Expression evaluation mismatch vs Presto
  File: velox/expression/tests/ExpressionVerifier.cpp:475

Instance 2 (seed=730604412): Expression evaluation mismatch vs Presto

Instance 3 (seed=630078507): Expression evaluation mismatch vs Presto

Instance 4 (seed=179380940): Expression evaluation mismatch vs Presto

⚪ Join Fuzzer — FUZZER Failure View logs

1 of 4 instances crashed (failed seed: 377890932; seeds 962015200, 1036667219, 956565828 passed)

Error: Velox and Reference results don't match at JoinFuzzer.cpp:740

Instance 2 (seed=377890932): Join results mismatch vs Presto
  Multiple "Getting spill input after join is aborted" errors (HashJoinBridge.cpp:372)
  File: velox/exec/fuzzer/JoinFuzzer.cpp:740

⚪ Window Fuzzer with Presto as source of truth — FUZZER Failure View logs

3 of 4 instances crashed (failed seeds: 673996794, 490174137, 229164968; seed 1019969253 passed)

Error: Velox and reference DB results don't match at WindowFuzzer.cpp:802

Instance 1 (seed=673996794): Window function results mismatch vs Presto
  File: velox/exec/fuzzer/WindowFuzzer.cpp:802

Instance 3 (seed=490174137): Window function results mismatch vs Presto

Instance 4 (seed=229164968): Window function results mismatch vs Presto
  "integer overflow" from Presto before failure

Correlation with PR changes:

🟢 These failures are NOT related to the PR changes. PR feat: Allow custom S3 filesystem factory #17400 modifies only S3 filesystem registration code (RegisterS3FileSystem.cpp, RegisterS3FileSystem.h, and S3FileSystemRegistrationTest.cpp) to add a pluggable S3FileSystemFactory. The fuzzer failures involve aggregation, expression evaluation, join, and window operations — completely unrelated subsystems.

Known issues:

⚠️ All 4 fuzzer jobs also fail on the main branch. The two most recent Fuzzer Jobs workflow runs on main (run 25841349965, run 25841343888) show the exact same 4 jobs failing: Aggregation Fuzzer, Expression Fuzzer with Presto SOT, Join Fuzzer, and Window Fuzzer.
These are pre-existing flaky failures, not regressions introduced by this PR.
Open issue #14308 tracks Presto fuzzer test result mismatches.

Reproduce locally:

# Aggregation Fuzzer (example with seed 119425723)
./velox_aggregation_fuzzer_test --seed 119425723 --duration_sec 300 --minloglevel=0 --stderrthreshold=2 --enable_oom_injection --presto_url http://127.0.0.1:8080

# Expression Fuzzer (example with seed 564466436)
./velox_expression_fuzzer_test --seed 564466436 --enable_variadic_signatures --velox_fuzzer_enable_complex_types --lazy_vector_generation_ratio 0.2 --presto_url http://127.0.0.1:8080

# Join Fuzzer (example with seed 377890932)
./velox_join_fuzzer --seed 377890932 --duration_sec 300 --minloglevel=0 --stderrthreshold=2 --enable_oom_injection --presto_url http://127.0.0.1:8080

# Window Fuzzer (example with seed 673996794)
./velox_window_fuzzer_test --seed 673996794 --duration_sec 300 --batch_size=50 --minloglevel=0 --stderrthreshold=2 --enable_oom_injection --presto_url http://127.0.0.1:8080

Recommended fix: No action needed for this PR. These are pre-existing fuzzer flakes on main and are unrelated to the S3 filesystem factory changes in this PR.

rui-mo · 2026-05-15T02:59:01Z

        initializeS3(logLevel, logLocation);
-        auto fs = std::make_shared<S3FileSystem>(bucketName, properties);
+        auto fs = fileSystemFactory
+            ? fileSystemFactory(std::move(bucketName), properties)


Please check fileSystemFactory is not null before calling it.

Allow custom S3 filesystem factory

31ed794

ReemaAlzaid requested a review from majetideepak as a code owner May 4, 2026 15:29

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 May 4, 2026

Merge branch 'main' into s3-registration-factory-hook

1e41a96

ReemaAlzaid mentioned this pull request May 4, 2026

Extend S3 filesystem support in Gluten apache/gluten#12022

Open

ReemaAlzaid requested a review from rui-mo May 5, 2026 17:53

ReemaAlzaid added 2 commits May 5, 2026 20:53

Merge branch 'main' into s3-registration-factory-hook

3c2c9b1

Merge branch 'main' into s3-registration-factory-hook

bab0b8a

JkSelf reviewed May 12, 2026

View reviewed changes

Comment thread velox/connectors/hive/storage_adapters/s3fs/tests/S3FileSystemRegistrationTest.cpp Outdated

JkSelf reviewed May 12, 2026

View reviewed changes

Comment thread velox/connectors/hive/storage_adapters/s3fs/tests/S3FileSystemRegistrationTest.cpp Outdated

JkSelf approved these changes May 12, 2026

View reviewed changes

ReemaAlzaid changed the title ~~Allow custom S3 filesystem factory~~ feat: Allow custom S3 filesystem factory May 12, 2026

address comments

8f25b5b

ReemaAlzaid requested a review from JkSelf May 12, 2026 13:19

JkSelf approved these changes May 12, 2026

View reviewed changes

rui-mo requested a review from Copilot May 13, 2026 01:48

Copilot started reviewing on behalf of rui-mo May 13, 2026 01:49 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

rui-mo reviewed May 13, 2026

View reviewed changes

ReemaAlzaid and others added 2 commits May 13, 2026 21:55

Potential fix for pull request finding

17d1401

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Address S3 factory review comments

956a515

ReemaAlzaid requested a review from rui-mo May 14, 2026 14:10

Merge branch 'main' into s3-registration-factory-hook

19ebbe3

rui-mo reviewed May 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow custom S3 filesystem factory#17400

feat: Allow custom S3 filesystem factory#17400
ReemaAlzaid wants to merge 8 commits into
facebookincubator:mainfrom
ReemaAlzaid:s3-registration-factory-hook

ReemaAlzaid commented May 4, 2026

Uh oh!

netlify Bot commented May 4, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 4, 2026 •

edited

Loading

Directly changed (9)

Transitively affected (5)

Uh oh!

Uh oh!

Uh oh!

JkSelf left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

rui-mo left a comment

Uh oh!

rui-mo May 13, 2026

Uh oh!

rui-mo May 13, 2026

Uh oh!

ReemaAlzaid May 14, 2026

Uh oh!

github-actions Bot commented May 14, 2026 •

edited

Loading

Uh oh!

rui-mo May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ReemaAlzaid commented May 4, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing change?

How was this patch tested?

Uh oh!

netlify Bot commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

github-actions Bot commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Build Impact Analysis

Selective Build Targets (building these covers all 14 affected)

Directly changed (9)

Transitively affected (5)

Uh oh!

Uh oh!

Uh oh!

JkSelf left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

rui-mo left a comment

Choose a reason for hiding this comment

Uh oh!

rui-mo May 13, 2026

Choose a reason for hiding this comment

Uh oh!

rui-mo May 13, 2026

Choose a reason for hiding this comment

Uh oh!

ReemaAlzaid May 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Failure Analysis

⚪ Aggregation Fuzzer with Presto as source of truth — FUZZER Failure View logs

⚪ Expression Fuzzer with Presto SOT — FUZZER Failure View logs

⚪ Join Fuzzer — FUZZER Failure View logs

⚪ Window Fuzzer with Presto as source of truth — FUZZER Failure View logs

Uh oh!

rui-mo May 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented May 4, 2026 •

edited

Loading

github-actions Bot commented May 4, 2026 •

edited

Loading

github-actions Bot commented May 14, 2026 •

edited

Loading