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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-4084][CORE][VL] Verify supported file system #4086

Closed
wants to merge 3 commits into from
Closed

[GLUTEN-4084][CORE][VL] Verify supported file system #4086

wants to merge 3 commits into from

Conversation

wForget
Copy link
Member

@wForget wForget commented Dec 18, 2023 •
edited
Loading

What changes were proposed in this pull request?

Verify supported file system

Fixes: #4084

How was this patch tested?

added test.

After this pr:

unsupported file system:

image
image
image

supported file system:

image

@github-actions GitHub Actions
Copy link

#4084

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@wForget wForget changed the title [GLUTEN-4084][VL] Verify supported file system [GLUTEN-4084][CORE][VL] Verify supported file system Dec 18, 2023
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@wForget wForget marked this pull request as ready for review December 18, 2023 09:43
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

2 similar comments
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@PHILO-HE
Copy link
Contributor

@rui-mo, do you have any comment?

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

rui-mo
rui-mo reviewed Dec 20, 2023
View reviewed changes
Copy link
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Just two quesitons.

backends-velox/src/main/scala/io/glutenproject/backendsapi/velox/VeloxBackend.scala Outdated Show resolved Hide resolved
backends-velox/src/main/scala/io/glutenproject/backendsapi/velox/VeloxBackend.scala Outdated
@@ -307,4 +350,6 @@ object BackendSettings extends BackendSettingsApi {
override def staticPartitionWriteOnly(): Boolean = true

override def allowDecimalArithmetic: Boolean = SQLConf.get.decimalOperationsAllowPrecisionLoss

override def requiredRootPaths(): Boolean = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this variable, and in what case the root paths are not required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need this variable, and in what case the root paths are not required?

I'm not familiar with clickhouse backend so I kept this and enabled it only for velox backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zzcclp Could you help check if it's needed for CH backend? Thanks.

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

2 similar comments
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@wForget wForget requested a review from PHILO-HE December 27, 2023 01:45
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@wForget
Copy link
Member Author

wForget commented Dec 29, 2023

@PHILO-HE @rui-mo could you please take a look again if you have time?

@PHILO-HE
Copy link
Contributor

Thanks so much for the iterations! Can we do the validation in SubstraitToVeloxPlanValidator? I think it would be better.

@wForget
Copy link
Member Author

wForget commented Jan 14, 2024

Thanks so much for the iterations! Can we do the validation in SubstraitToVeloxPlanValidator? I think it would be better.

Thanks for your guidance I will try that.

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 4, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale stale label Mar 4, 2024
@github-actions GitHub Actions
Copy link

This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.

@github-actions github-actions bot closed this Mar 15, 2024
@zhli1142015
Copy link
Contributor

zhli1142015 commented Jul 15, 2024 •
edited
Loading

Hello @wForget,
I saw this PR is closed, will you continue this work?
Thanks.

@wForget
Copy link
Member Author

wForget commented Jul 15, 2024

Hello @wForget, I saw this PR is closed, will you continue this work? Thanks.

I disabled columnar filescan by adding spark.gluten.sql.columnar.filescan=false; config, so this PR is unnecessary for me, if this is useful I can continue to work on it. cc @PHILO-HE WDYT?

@zhli1142015
Copy link
Contributor

We need this PR, when reading from for not supported file systems, falling back is always better than failing.

@PHILO-HE
Copy link
Contributor

Hello @wForget, I saw this PR is closed, will you continue this work? Thanks.

I disabled columnar filescan by adding spark.gluten.sql.columnar.filescan=false; config, so this PR is unnecessary for me, if this is useful I can continue to work on it. cc @PHILO-HE WDYT?

@wForget, do you remember the landing gap of this pr? Thanks!

@wForget
Copy link
Member Author

wForget commented Jul 19, 2024

@wForget, do you remember the landing gap of this pr? Thanks!

I remember the current status should be waiting for review, but I didn't move forward with it because I found spark.gluten.sql.columnar.filescan=false configuration. Do we need to reopen this PR?

@PHILO-HE
Copy link
Contributor

@wForget, do you remember the landing gap of this pr? Thanks!

I remember the current status should be waiting for review, but I didn't move forward with it because I found spark.gluten.sql.columnar.filescan=false configuration. Do we need to reopen this PR?

@wForget, I see. Thanks for your work!
@zhli1142015, could you take over this work if it is really necessary to you? You can create a new pr based on this one, with @wForget's work highlighted there.

@zhli1142015
Copy link
Contributor

@wForget, do you remember the landing gap of this pr? Thanks!

I remember the current status should be waiting for review, but I didn't move forward with it because I found spark.gluten.sql.columnar.filescan=false configuration. Do we need to reopen this PR?

@wForget, I see. Thanks for your work! @zhli1142015, could you take over this work if it is really necessary to you? You can create a new pr based on this one, with @wForget's work highlighted there.

Sure, thanks @wForget 's work.

@zhli1142015 zhli1142015 mentioned this pull request Jul 29, 2024
Closed
@wForget wForget mentioned this pull request Aug 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rui-mo rui-mo rui-mo left review comments

@zhztheplayer zhztheplayer zhztheplayer left review comments

@zhli1142015 zhli1142015 zhli1142015 left review comments

@leesf leesf leesf left review comments

@PHILO-HE PHILO-HE Awaiting requested review from PHILO-HE

Assignees
No one assigned
Labels
stale stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] Verify supported file system
6 participants
@wForget @PHILO-HE @zhli1142015 @leesf @zhztheplayer @rui-mo