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-5414] [VL] Move ArrowFileScanExec class to module backends-velox #5667

Merged
merged 1 commit into from May 13, 2024
Merged

[GLUTEN-5414] [VL] Move ArrowFileScanExec class to module backends-velox #5667

merged 1 commit into from May 13, 2024

Conversation

jinchengchenghh
Copy link
Contributor

@jinchengchenghh jinchengchenghh commented May 9, 2024 •
edited

Arrow related class should not be in module gluten-core

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

#5414

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

@zhztheplayer
Copy link
Member

@jinchengchenghh Thanks for keeping improving the code.

My suggestion: Let's see if it's feasible to directly move ArrowFileSourceScanExec into module backends-velox.

Then move this part of code into a individual physical rule plugged into Gluten through this API.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

@jinchengchenghh
Copy link
Contributor Author

/Benchmark Velox

@jinchengchenghh
Copy link
Contributor Author

https://github.com/apache/incubator-gluten/blob/main/gluten-core/src/main/scala/org/apache/gluten/utils/PlanUtil.scala#L53

If I move ArrowFileSourceScanExec to backends-velox, here I won't access it

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

@jinchengchenghh
Copy link
Contributor Author

And the genExtendedColumnarValidationRules may take effect after add c2r r2c, so the test failed

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jinchengchenghh jinchengchenghh changed the title [GLUTEN-5414] [VL] Move ArrowCsvFileFormat class match to its module [GLUTEN-5414] [VL] Move ArrowCsvFileFormat class match to its module and enable arrow memory pool track May 10, 2024
@zhztheplayer
Copy link
Member

https://github.com/apache/incubator-gluten/blob/main/gluten-core/src/main/scala/org/apache/gluten/utils/PlanUtil.scala#L53

If I move ArrowFileSourceScanExec to backends-velox, here I won't access it

Am I missing something? I assume ArrowFileSourceScanExec should return true for #outputNativeColumnarData?

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jinchengchenghh jinchengchenghh changed the title [GLUTEN-5414] [VL] Move ArrowCsvFileFormat class match to its module and enable arrow memory pool track [GLUTEN-5414] [VL] Move ArrowCsvFileFormat class match to its module May 10, 2024
@jinchengchenghh
Copy link
Contributor Author

Can you help review this one again? Thanks! @zhztheplayer

@jinchengchenghh jinchengchenghh changed the title [GLUTEN-5414] [VL] Move ArrowCsvFileFormat class match to its module [GLUTEN-5414] [VL] Move ArrowFileScanExec class to module backends-velox May 11, 2024
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@zhztheplayer zhztheplayer merged commit 09950de into apache:main May 13, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhztheplayer zhztheplayer zhztheplayer approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@jinchengchenghh @zhztheplayer