[CORE] Rename TransformHint to FallbackTag

[gaoyangxiaozhu]

What changes were proposed in this pull request?

A follow up PR of #4731

(Fixes: #6246)

Short details for main change:

Class name

TransformHit -> FallbackTag
TransformHits -> FallbackTags

Method

isNotTransformable -> nonEmpty
maybeTransformable -> maybeOffloadable
tagNotTransformable -> add
tagAllNotTransformable -> addRecursively
getHint -> getTag
getHintOption -> getTagOption

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)

[github-actions]

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?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

• Other pull requests

[github-actions]

Run Gluten Clickhouse CI

[github-actions]

Run Gluten Clickhouse CI

[xumingming]

After the change, the API of this class is:

FallbackHints.tagNotTransformable

It has two concepts: Fallback and Transformable here. How about:

FallbackHints.tagFallback

To reduce the concept count.

[github-actions]

Run Gluten Clickhouse CI

[gaoyangxiaozhu]

After the change, the API of this class is:

FallbackHints.tagNotTransformable

It has two concepts: Fallback and Transformable here. How about:

FallbackHints.tagFallback

To reduce the concept count.

actually i think FallbackHints.tagNotTransformable is more readable (people can easy to know what it means) than FallbackHints.tagFallback, but those 2 options both ok for me.

@zhztheplayer for input

[github-actions]

Run Gluten Clickhouse CI

[gaoyangxiaozhu]

Run Gluten Clickhouse CI

[gaoyangxiaozhu]

@zhztheplayer / @rui-mo / @PHILO-HE help review also for this rename change

[zhztheplayer]

I would suggest using brand-new names to replace current.

Can we do these renaming:

1. TransformHints.maybeTransformable(p) -> OffloadErrors.empty(p)
2. TransformHints.isNotTransformable(p) -> OffloadErrors.hasErrors(p)

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.

[gaoyangxiaozhu]

I would suggest using brand-new names to replace current.

Can we do these renaming:

1. TransformHints.maybeTransformable(p) -> OffloadErrors.empty(p)
2. TransformHints.isNotTransformable(p) -> OffloadErrors.hasErrors(p)

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 *Transform* API of the hit class, you also want to rename the class TransformHits to OffloadErrors instead of FallbackHits ?

[zhztheplayer]

That's right. My feeling is to rename both the class and the methods.

[gaoyangxiaozhu]

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 FallbackHits and OffloadErrors, I personally still prefer FallbackHits. OffloadErrors for me is a bit too much, and seems a bit misleading, as people at first glance may thought it for a central place for handling all actual offload fail errors to show to users, which isn't the case. Moreover, it doesn't a good naming to effectively reflect moving computation to native.

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, FallbackHits is a more suitable name compared to OffloadErrors.

As for the proposed APIs - empty and hasErrors has same problems when developer use it in if condition statement to check if a plan is offloadable or not, as they are not immediately clear. People might wonder what empty and Errors specifically refer to.

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. :)

[gaoyangxiaozhu]

@rui-mo / @PHILO-HE / @FelixYBW / @xumingming for more inputs of renaming part.

[github-actions]

Run Gluten Clickhouse CI

[zhztheplayer]

Thank you for the detailed thoughts. It's convincing to me since I haven't had a strong inclination between OffloadErrors and something like FallbackTag so long as we can remove transformable stuffs. You can use the latter as long as you can reason about why it's better.

BTW I have some comments for your list:

addTransformableTags -> addTagsIfPlanFallback

Could you check if we can use transformUp or foreachUp ? If yes it's just OK to rename addTransformableTag to apply0 then call it in lambda.

tagAllNotTransformable -> addFallbackTag

Suggestion: addFallbackTag FallbackTags.add

tagAllNotTransformable -> addFallbackTags

Suggestion: addFallbackTags FallbackTags.addRecursively

maybeTransformable -> emptyTags

Could still have a method maybeOffloadable, which just redirects calls to a private method FallbackTags.empty

[gaoyangxiaozhu]

Thank you for the detailed thoughts. It's convincing to me since I haven't had a strong inclination between OffloadErrors and something like FallbackTag so long as we can remove transformable stuffs. You can use the latter as long as you can reason about why it's better.

BTW I have some comments for your list:

addTransformableTags -> addTagsIfPlanFallback

Could you check if we can use transformUp or foreachUp ? If yes it's just OK to rename addTransformableTag to apply0 then call it in lambda.

tagAllNotTransformable -> addFallbackTag

Suggestion: addFallbackTag FallbackTags.add

tagAllNotTransformable -> addFallbackTags

Suggestion: addFallbackTags FallbackTags.addRecursively

maybeTransformable -> emptyTags

Could still have a method maybeOffloadable, which just redirect calls to a private method FallbackTags.empty

thank you! Let me do refactor according to your suggestion.

[github-actions]

Run Gluten Clickhouse CI

[github-actions]

Run Gluten Clickhouse CI

[github-actions]

Run Gluten Clickhouse CI

[github-actions]

Run Gluten Clickhouse CI

[gaoyangxiaozhu]

Run Gluten Clickhouse CI

[gaoyangxiaozhu]

@zhztheplayer for help review. thanks

[zhztheplayer]

Thanks!