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] Separate each jni wrapper to different files #2845

Closed
Yohahaha opened this issue Aug 22, 2023 · 6 comments
Closed

[VL] Separate each jni wrapper to different files #2845

Yohahaha opened this issue Aug 22, 2023 · 6 comments
Labels
enhancement New feature or request stale stale

Comments

@Yohahaha
Copy link
Contributor

Yohahaha commented Aug 22, 2023

Description

JniWrapper.cc is too large and may be larger in the future, we could separate it into sub dirs when some stateful static variables be removed, and get benefits from readability and UT.

@Yohahaha
Copy link
Contributor Author

After issue 3077 completed, I will separate current JniWrapper.cc into different sub-dir, it will looks like below:

cpp
  |- jni
  |- memory
  |- compute
    |- r2c
      |- R2CJniWrapper.cc
      |- Row2Columnar.h
      |- test
      |- benchmark
    |- c2r
      |- C2RJniWrapper.cc
      |- Columnar2Row.h
      |- ...
    |- shuffle
      |- ShuffleReaderJniWrapper.cc
      |- ShuffleReader.h
      |- ShuffleWriterJniWrapper.cc
      |- ShuffleWriter.h
      |- ...
    |- serializer
    |- wholestage
    |- [common files]
  |- ...

Any suggestions? @jinchengchenghh @zhztheplayer @rui-mo

@zhztheplayer
Copy link
Member

This sort of refactor is needed.

The thing is that we should find a proper time to apply the change, since it may easily conflict with other pending PRs.

@Yohahaha
Copy link
Contributor Author

The thing is that we should find a proper time to apply the change, since it may easily conflict with other pending PRs.

Good point, I prefer do the refactor in weekend, before that, let's discuss the structure we expected.

@Yohahaha
Copy link
Contributor Author

Also cc @ulysses-you @FelixYBW @zhouyuan

@ulysses-you
Copy link
Contributor

I think make jni file name align with java class is good. The VexloJniWrapper should also be taken into account. e.g., The java class PlanEvaluatorJniWrapper has several jni methods that map to both JniWrapper and VexloJniWrapper. What's the really difference between VexloJniWrapper and JniWrapper ?

@zhztheplayer
Copy link
Member

The java class PlanEvaluatorJniWrapper has several jni methods that map to both JniWrapper and VexloJniWrapper.

PlanEvaluatorJniWrapper -> VeloxJniWrapper

Such mappings should be finally eliminated IIUC. We already had Backend and ExecutionCtx as the APIs across backends.

@github-actions github-actions bot added the stale stale label Nov 17, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale stale
Projects
None yet
Development

No branches or pull requests

3 participants