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

[GLUTEN-2845][VL] Separate jni method into different files #3396

Closed
wants to merge 4 commits into from

Conversation

Yohahaha
Copy link
Contributor

@Yohahaha Yohahaha commented Oct 12, 2023

Changes:

  1. Split methods of JniWrapper.cc into different .cc files, each new file's name is aligned with java file, only do movement without logic change. Keep nativeValidateWithFailureReason in VeloxJniWrapper, will refactor in follow up patch.
  2. Move static jclass/jmethodId to JniCommonState.

After this patch, core dir structure will like below:

core
├── compute
│   ├── ExecutionCtxJniWrapper.cc
│   ├── PlanEvaluatorJniWrapper.cc
│   ├── batch
│   │   ├── ColumnarBatchJniWrapper.cc
│   │   └── ColumnarBatchOutIterJniWrapper.cc
│   ├── c2r
│   │   └── ColumnarToRowJniWrapper.cc
│   ├── r2c
│   │   └── RowToColumnarJniWrapper.cc
│   ├── serializer
│   │   └── ColumnarBatchSerializerJniWrapper.cc
│   ├── shuffle
│   └── rss
│   │   ├── ShuffleJniWrapper.cc
│   └── writer
│       └── DatasourceJniWrapper.cc
├── config
├── jni
├── memory
│   ├── MemoryJniWrapper.cc

@github-actions
Copy link

#2845

@Yohahaha Yohahaha changed the title [GLUTEN-2845][VL] Separate jni method into different files [WIP][GLUTEN-2845][VL] Separate jni method into different files Oct 13, 2023
@Yohahaha
Copy link
Contributor Author

Yohahaha commented Oct 13, 2023

This patch is ready, please help review, thanks! @zhztheplayer @ulysses-you @PHILO-HE

@Yohahaha Yohahaha changed the title [WIP][GLUTEN-2845][VL] Separate jni method into different files [GLUTEN-2845][VL] Separate jni method into different files Oct 13, 2023
@kecookier
Copy link
Contributor

Why do this?

@zuochunwei
Copy link
Contributor

don't use a deep directory, we made this adjustment one year ago and added a rule in the cpp programming guide.
CPP is different from Java.

@Yohahaha
Copy link
Contributor Author

don't use a deep directory, we made this adjustment one year ago and added a rule in the cpp programming guide. CPP is different from Java.

Main purpose is split JniWrapper, I prefer talk design in issue.

What the difference between cpp/operators/c2r and cpp/compute/c2r ? does second increase directory level?

@zhztheplayer
Copy link
Member

Probably the change is too big to match the volume of improvement it brings. Let's hold it off until the time is ripe? To avoid blocking other's pending work. @Yohahaha What do you think?

@Yohahaha
Copy link
Contributor Author

Probably the change is too big to match the volume of improvement it brings. Let's hold it off until the time is ripe? To avoid blocking other's pending work. @Yohahaha What do you think?

This patch is not urgent but needed, JniWrapper.cc already has 1372 lines, we need a big patch to split it and make jni layer more clean and readable.
Which time is your expected? @zhztheplayer I think anytime is good time if we start to do this "dirty" work, it will bring help for beginners.

@zhztheplayer
Copy link
Member

zhztheplayer commented Oct 13, 2023

JniWrapper.cc already has 1372 lines

It is big but somewhat still organized. I think the complexity of maitaining it in future is basically predictable.

I agree that this sort of change improves the code but we should consider about the cost of applying the change. More concretely, a considerable amount of rebasing actions will be needed in others' work if we merged the PR, also a lot of code lines' git author credit will be lost. So the cost of applying this change is high at the moment.

@Yohahaha
Copy link
Contributor Author

JniWrapper.cc already has 1372 lines

It is big but somewhat still organized. I think the complexity of maitaining it in future is basically predictable.

I agree that this sort of change improves the code but we should consider about the cost of applying the change. More concretely, a considerable amount of rebasing work will be needed in others' work if we merged the PR, also a lot of code lines' git author credit will be lost. So the cost of applying this change is high at the moment.

Make sense for me. Thanks!

@zhztheplayer
Copy link
Member

I am closing this. Feel free to reopen when time is ripe.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants