Skip to content

Use legacySizeOfNull argument to determine the behavior of Spark size function#10100

Closed
philo-he wants to merge 8 commits intofacebookincubator:mainfrom
philo-he:fix-array-size
Closed

Use legacySizeOfNull argument to determine the behavior of Spark size function#10100
philo-he wants to merge 8 commits intofacebookincubator:mainfrom
philo-he:fix-array-size

Conversation

@philo-he
Copy link
Contributor

@philo-he philo-he commented Jun 7, 2024

  1. Spark size function's legacySizeOfNull is specified either by other functions
    (e.g., array_size function) or by configuration. However, in the existing
    implementation, it only depends on the configuration property. This pr removes
    the configuration property, then just uses the legacySizeOfNull arg passed from
    framework like Gluten.
  2. At Spark's analysis time, array_size is replaced with size function. And their
    implementations are same actually. This pr removes duplicate code.

@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 Jun 7, 2024
@netlify
Copy link

netlify bot commented Jun 7, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9b5797a
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/667b6da8720eb900088f3647

@philo-he philo-he changed the title Fix Spark array_size function Use passed arg to determine legacySizeOfNull behavior of Spark size function Jun 7, 2024
@philo-he philo-he changed the title Use passed arg to determine legacySizeOfNull behavior of Spark size function Use passed arg to determine legacySizeOfNull behavior for Spark size function Jun 14, 2024
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!

@philo-he
Copy link
Contributor Author

@rui-mo, do you have any comment?

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!

@philo-he
Copy link
Contributor Author

@rui-mo, do you have any other comment?

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. Can we make the title more concise maybe like Use legacySizeOfNull parameter to determine the behavior of Spark size function?

@philo-he philo-he changed the title Use passed arg to determine legacySizeOfNull behavior for Spark size function Use legacySizeOfNull argument to determine the behavior of Spark size function Jun 19, 2024
@philo-he
Copy link
Contributor Author

Hi @pedroerp, this pr is approved by @rui-mo. Do you have any comment?

@pedroerp
Copy link
Contributor

Thanks guys. @rui-mo I forgot to mention, but please add the "ready-to-merge" tag to PRs which are reviewed and ready to be merged. This will alert our oncall.

@pedroerp pedroerp added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jun 20, 2024
@rui-mo
Copy link
Collaborator

rui-mo commented Jun 20, 2024

@pedroerp Thank you for the pointer. I understand.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

bikramSingh91 added a commit to bikramSingh91/presto that referenced this pull request Jun 20, 2024
Remove use of spark specific query config which is going to be
removed in  facebookincubator/velox#10100
@bikramSingh91
Copy link
Contributor

Created a PR in presto repo to remove the use of the query config that is removed in this PR. prestodb/presto#23046

@philo-he
Copy link
Contributor Author

philo-he commented Jun 21, 2024

Created a PR in presto repo to remove the use of the query config that is removed in this PR. prestodb/presto#23046

@bikramSingh91, thanks for your fix!

gggrace14 pushed a commit to prestodb/presto that referenced this pull request Jun 24, 2024
Remove use of spark specific query config which is going to be
removed in  facebookincubator/velox#10100
@philo-he
Copy link
Contributor Author

@bikramSingh91, can this pr be merged? The CI failure is related to network error.

@philo-he
Copy link
Contributor Author

@bikramSingh91, I just rebased this pr. All checks passed.

@philo-he
Copy link
Contributor Author

Friendly ping @bikramSingh91. Thanks!

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@bikramSingh91 merged this pull request in 258db51.

@conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit 258db516.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

philo-he added a commit to philo-he/velox that referenced this pull request Jul 1, 2024
… function (facebookincubator#10100)

Summary:
1) Spark `size` function's legacySizeOfNull is specified either by other functions
 (e.g., `array_size` function) or by configuration. However, in the existing
implementation, it only depends on the configuration property. This pr removes
the configuration property, then just uses the legacySizeOfNull arg passed from
framework like Gluten.
2) At Spark's analysis time, `array_size` is replaced with `size` function. And their
implementations are same actually. This pr removes duplicate code.

Pull Request resolved: facebookincubator#10100

Reviewed By: xiaoxmeng

Differential Revision: D58825758

Pulled By: bikramSingh91

fbshipit-source-id: 5e1c9679832dfb6ac7dd15e3c0c1d979d3219b93
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. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants