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

feat: Add Spark get_struct_field function #12166

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

WangGuangxin
Copy link
Contributor

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 24, 2025
Copy link

netlify bot commented Jan 24, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 437ea5c
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67a70042b667340009a30832

@WangGuangxin WangGuangxin force-pushed the feat_get_struct_field branch from eeebd22 to 713b61d Compare January 24, 2025 12:58
velox/functions/sparksql/specialforms/GetStructField.cpp Outdated Show resolved Hide resolved

namespace facebook::velox::functions::sparksql {

class GetStructFieldExpr : public SpecialForm {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you need to extend class FunctionCallToSpecialForm rather than SpecialForm, so as the implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are different, one is for function register, the other is for function iteself

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can implement the get_struct_field as a vector function then register it as a special form. What's the specific consideration here to implement it by extending SpecialForm?

@WangGuangxin
Copy link
Contributor Author

@jinchengchenghh @rui-mo Comments addressed. Please help review this again when you are convenient

@rui-mo rui-mo changed the title feat: Add spark get_struct_field function feat: Add Spark get_struct_field function Feb 11, 2025
Copy link
Collaborator

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Please also add documentation for this function.

VectorPtr ordinalVector;
inputs_[0]->eval(rows, context, input);
inputs_[1]->eval(rows, context, ordinalVector);
VELOX_USER_CHECK(ordinalVector->isConstantEncoding());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can move the checks inside constructSpecialForm and extract the constant value there. Some example in decimal round:

VELOX_USER_CHECK_EQ(
args[1]->type()->kind(),
TypeKind::INTEGER,
"The second argument of decimal_round should be of integer type.");
auto constantExpr = std::dynamic_pointer_cast<exec::ConstantExpr>(args[1]);
VELOX_USER_CHECK_NOT_NULL(
constantExpr,
"The second argument of decimal_round should be constant expression.");
VELOX_USER_CHECK(
constantExpr->value()->isConstantEncoding(),
"The second argument of decimal_round should be wrapped in constant vector.");
auto constantVector =
constantExpr->value()->asUnchecked<ConstantVector<int32_t>>();
VELOX_USER_CHECK(
!constantVector->isNullAt(0),
"The second argument of decimal_round is non-nullable.");
scale = constantVector->valueAt(0);


namespace facebook::velox::functions::sparksql {

class GetStructFieldExpr : public SpecialForm {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can implement the get_struct_field as a vector function then register it as a special form. What's the specific consideration here to implement it by extending SpecialForm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants