Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VL] Fall back scan if file scheme is not supported by registered file systems #6672

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

zhli1142015
Copy link
Contributor

@zhli1142015 zhli1142015 commented Aug 1, 2024

What changes were proposed in this pull request?

Fallback scan if root path is not supported by registered file systems

Fixes: #6620

How was this patch tested?

UT.

Copy link

github-actions bot commented Aug 1, 2024

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Copy link

github-actions bot commented Aug 1, 2024

Run Gluten Clickhouse CI

@zhli1142015
Copy link
Contributor Author

cc @PHILO-HE , thanks.

@PHILO-HE
Copy link
Contributor

PHILO-HE commented Aug 5, 2024

@wForget, could you take a look if you have time?

@wForget
Copy link
Member

wForget commented Aug 6, 2024

@zhli1142015 Thanks for your work, as comment in #4086 (comment) suggested putting the validation into SubstraitToVeloxPlanValidator, I also tried it in the original PR, can you confirm which way is better?

@zhli1142015
Copy link
Contributor Author

zhli1142015 commented Aug 7, 2024

Yes. There are lots of change sinse your PR. setSplitInfos method is removed from the SubstraitContext. If we want to still follow that way, we need to bing it back or add separate property for this.
And getSplitInfos is an expensive call, we should avoid to use it in validation. Instead we retrieve the first root path for validation here.
And looks all file related validation is kept in the method supportFileFormatRead, file format, schema, etc. So we put the check here also.
Last, FakeParentPathFileSystem doesn't work as expected in my local UT runs, mockfs:// is not supported by local file system. so I think a separate JNI wrapper API is needd for UT.
Hee are the differences in my mind, please le me know if this makes sense to you, @PHILO-HE and @wForget , thanks.

Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

@zhli1142015, just posted some comments. Thanks!

Copy link

github-actions bot commented Aug 8, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 8, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Aug 8, 2024

Run Gluten Clickhouse CI

@zhli1142015 zhli1142015 requested review from PHILO-HE and wForget August 8, 2024 11:56
@PHILO-HE PHILO-HE changed the title [VL] Fallback scan if root path is not supported by registered file systems [VL] Fall back scan if file scheme is not supported by registered file systems Aug 8, 2024
@wForget
Copy link
Member

wForget commented Aug 9, 2024

Thanks, overall LGTM.

@PHILO-HE
Copy link
Contributor

@zhli1142015, could you respond to @wForget's comments?

Copy link

Run Gluten Clickhouse CI

@zhli1142015
Copy link
Contributor Author

Just updated, thanks.

Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Thanks!

@PHILO-HE PHILO-HE merged commit ba3318e into apache:main Aug 12, 2024
9 of 10 checks passed
sharkdtu pushed a commit to sharkdtu/gluten that referenced this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VL] Fallback scan when reading from unsupported file system
3 participants