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] Fallback happens if child node of GetStructField is Scalar Function Node #8306

Open
ayushi-agarwal opened this issue Dec 23, 2024 · 10 comments · May be fixed by #8606
Open

[VL] Fallback happens if child node of GetStructField is Scalar Function Node #8306

ayushi-agarwal opened this issue Dec 23, 2024 · 10 comments · May be fixed by #8606
Labels
enhancement New feature or request

Comments

@ayushi-agarwal
Copy link
Contributor

Description

Enhance https://github.com/apache/incubator-gluten/blob/main/backends-velox/src/main/scala/org/apache/gluten/expression/ExpressionTransformer.scala#L54 to add support for child node type as Scalar Function Node:

Example:
24/12/23 10:00:23 WARN GlutenFallbackReporter: Validation failed for plan: Filter[QueryId=4], due to: Unsupported child expression of GetStructField: from_json(StructField(platformId,StringType,true), json_column#7, Some(America/Los_Angeles)).platformId

More reference here:
#4039 (comment)

@rui-mo
Copy link
Contributor

rui-mo commented Dec 23, 2024

@ayushi-agarwal Thanks for reporting this issue! I understand the case now and will spare some time to investigate it this or next week.

@FelixYBW FelixYBW changed the title Fallback happens if child node of GetStructField is Scalar Function Node [VL] Fallback happens if child node of GetStructField is Scalar Function Node Dec 23, 2024
@WangGuangxin
Copy link
Contributor

@rui-mo we need to add a GetStructField scalar function in velox side. I would like to work on this if you have not started yet

@rui-mo
Copy link
Contributor

rui-mo commented Jan 2, 2025

@WangGuangxin We probably don't need a scalar function implementation in Velox as it already provides 'FieldAccessTypedExpr' and 'DereferenceTypedExpr' for this purpose. I assume we need to investigate how to create such expression when the child is a scalar function. In Gluten, 'SelectionNode' contains a nested child index parameter, which will be converted as 'FieldAccessTypedExpr' with input.

https://github.com/facebookincubator/velox/blob/d97bae061265e78194cde11f4132714f7c01e44d/velox/core/Expressions.h#L292-L298

https://github.com/facebookincubator/velox/blob/d97bae061265e78194cde11f4132714f7c01e44d/velox/core/Expressions.h#L393-L395

@WangGuangxin
Copy link
Contributor

when

@rui-mo We also encountered this fallback in our production env, what we do here at first is PreProject if there is GetStructFiled to make sure the input is always an attribute.

@rui-mo
Copy link
Contributor

rui-mo commented Jan 2, 2025

what we do here at first is PreProject if there is GetStructFiled to make sure the input is always an attribute.

Hi @WangGuangxin, thanks for sharing this information! I would like to know if you have encountered any problems with this approach that have made you consider replacing it with a scalar function.

@WangGuangxin
Copy link
Contributor

what we do here at first is PreProject if there is GetStructFiled to make sure the input is always an attribute.

Hi @WangGuangxin, thanks for sharing this information! I would like to know if you have encountered any problems with this approach that have made you consider replacing it with a scalar function.

@rui-mo In fact it works very well by PreProject except there are many Projects

@rui-mo
Copy link
Contributor

rui-mo commented Jan 8, 2025

hi @WangGuangxin, do you want to take the work to implement GetStructField function in Velox? I assume this can simplify the implementation in Gluten and does not introduce extra pre-projections. I suppose we need to register the get_struct_field function as a special form in Velox since the result type cannot be inferred from the input types.

@WangGuangxin
Copy link
Contributor

hi @WangGuangxin, do you want to take the work to implement GetStructField function in Velox? I assume this can simplify the implementation in Gluten and does not introduce extra pre-projections. I suppose we need to register the get_struct_field function as a special form in Velox since the result type cannot be inferred from the input types.

@rui-mo sure

@ayushi-agarwal
Copy link
Contributor Author

@WangGuangxin Do you have any patch ready for this? I tried doing pre-project but it doesn't work for some cases.

@WangGuangxin
Copy link
Contributor

@WangGuangxin Do you have any patch ready for this? I tried doing pre-project but it doesn't work for some cases.

@ayushi-agarwal you can try
#8606 and
facebookincubator/velox#12166

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

Successfully merging a pull request may close this issue.

3 participants