fix(cudf): Fix table scan fallback condition and library#14728
Closed
jinchengchenghh wants to merge 6 commits intofacebookincubator:mainfrom
Closed
fix(cudf): Fix table scan fallback condition and library#14728jinchengchenghh wants to merge 6 commits intofacebookincubator:mainfrom
jinchengchenghh wants to merge 6 commits intofacebookincubator:mainfrom
Conversation
jinchengchenghh
requested review from
bdice,
devavret,
karthikeyann and
mhaseeb123
as code owners
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
mhaseeb123
requested changes
Sep 5, 2025
Comment on lines
+60
to
+67
| VELOX_CHECK( | ||
| start <= | ||
| static_cast<uint64_t>(std::numeric_limits<cudf::size_type>::max()), | ||
| "ParquetConnectorSplit `start` must be less than or equal to 2^31"); | ||
| VELOX_CHECK( | ||
| length <= | ||
| static_cast<uint64_t>(std::numeric_limits<cudf::size_type>::max()), | ||
| "ParquetConnectorSplit `length` must be less than or equal to 2^31"); |
Collaborator
There was a problem hiding this comment.
Shouldn't start and length be set from constructor args like in #14129. I believe these changes are duplicate as well
Collaborator
Author
There was a problem hiding this comment.
Yes, I just move it from header file to cpp file, the #14129. should be merged first
jinchengchenghh
changed the title
jinchengchenghh
force-pushed
the
cudf_connector
branch
from
3d517d4 to
81a77af
Compare
Collaborator
Author
|
@mhaseeb123 Could you help review again? Thanks! |
mhaseeb123
approved these changes
Sep 19, 2025
Collaborator
|
LGTM thanks. |
Collaborator
Author
|
The red CI is not caused by this PR #14920 |
jinchengchenghh
added
the
ready-to-merge
Contributor
|
@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this in D83066640. |
Contributor
|
@xiaoxmeng merged this pull request in 62fe64a. |
jinchengchenghh
added a commit
to jinchengchenghh/velox
that referenced
this pull request
Sep 24, 2025
…ubator#14728) Summary: Gluten needs to link velox_cudf_parquet_connector, so it cannot be OBJECT Aligns with LocalFileSystem to extractFilePath https://github.com/facebookincubator/velox/blob/main/velox/common/file/FileSystems.cpp#L109-L121 Related PR: apache/gluten#10622 Resolves: facebookincubator#14730 Pull Request resolved: facebookincubator#14728 Reviewed By: zacw7 Differential Revision: D83066640 Pulled By: xiaoxmeng fbshipit-source-id: 8835a883b4c517b0097e3afc1c83ac3994a91f38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gluten needs to link velox_cudf_parquet_connector, so it cannot be OBJECT
Aligns with LocalFileSystem to extractFilePath https://github.com/facebookincubator/velox/blob/main/velox/common/file/FileSystems.cpp#L109-L121
Related PR: apache/gluten#10622
Resolves: #14730