-
Notifications
You must be signed in to change notification settings - Fork 376
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
[Core][WIP] Rename transformhit to fallbackhit #6254
base: main
Are you sure you want to change the base?
[Core][WIP] Rename transformhit to fallbackhit #6254
Conversation
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?
See also: |
Run Gluten Clickhouse CI |
1 similar comment
Run Gluten Clickhouse CI |
After the change, the API of this class is: FallbackHints.tagNotTransformable It has two concepts: FallbackHints.tagFallback To reduce the concept count. |
Run Gluten Clickhouse CI |
actually i think @zhztheplayer for input |
…gayangya/rename_transformhit_fallbackhit
Run Gluten Clickhouse CI |
1 similar comment
Run Gluten Clickhouse CI |
@zhztheplayer / @rui-mo / @PHILO-HE help review also for this rename change |
I would suggest using brand-new names to replace current. Can we do these renaming:
As a basis of Gluten, “Transform” / “Transformer”/ "Transformable" are all a little bit too general to describe about moving a computation to native. We use the names for legacy reason. And this is also why I suggest the above renaming. |
hey @zhztheplayer HongZe, don't sure if I get your point clearly, in addition to rename the |
That's right. My feeling is to rename both the class and the methods. |
Got it, Hongze! I totally agree with your point that "Transform", "Transformer", and "Transformable" are all a bit too general to describe moving a computation to native. Renaming both the class name and APIs is indeed the right approach. However, Regarding the proposed class names, between And let's back to Look at the class implementation, it primarily use to add UNSUPPORT_XXX tags to a Spark plan (without actually offloading computation with user data) after a simple validation with doValidation or other validation rules, that means the class do is not a actual offloading or transform converting but action to tag plans that can't be offloaded. Therefore, As for the proposed APIs - For renaming APIs parts, the following are my purposed new naming (there are currently 5 APIs with transform that need renaming), i think maybe better and straigtforword ? addTransformableTags -> addTagsIfPlanFallback
tagNotTransformable -> addFallbackTag
tagAllNotTransformable -> addFallbackTags
isNotTransformable -> hasFallbackTags
maybeTransformable -> emptyTags Let's discuss more here to finalize the final class name and APIs. :) |
@rui-mo / @PHILO-HE / @FelixYBW / @xumingming for more inputs of renaming part. |
Run Gluten Clickhouse CI |
Thank you for the detailed thoughts. It's convincing to me since I haven't had a strong inclination between BTW I have some comments for your list:
Could you check if we can use
Suggestion:
Suggestion:
Could still have a method |
…gayangya/rename_transformhit_fallbackhit
thank you! Let me do refactor according to your suggestion. |
Run Gluten Clickhouse CI |
What changes were proposed in this pull request?
A follow up PR of #4731
(Fixes: #6246)
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)