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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-2956][VL] Support Spark NullType #2996

Merged
merged 8 commits into from
Mar 1, 2024
Merged

[GLUTEN-2956][VL] Support Spark NullType #2996

merged 8 commits into from
Mar 1, 2024

Conversation

PHILO-HE
Copy link
Contributor

@PHILO-HE PHILO-HE commented Sep 1, 2023

What changes were proposed in this pull request?

Spark's NullType can be mapped to substrait's nothing type and velox's UnknownType. And we also need some code changes in Velox to support this type.

(Fixes: #2956)

How was this patch tested?

Existing Spark UTs & new UTs to be added.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

#2956

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 6, 2023

@jackylee-ch, please have a test to see whether there is any other issue.

@PHILO-HE PHILO-HE mentioned this pull request Sep 7, 2023
Closed
@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 8, 2023

/Benchmark Velox

@GlutenPerfBot
Copy link
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query log/native_2996_time.csv log/native_master_09_07_2023_192f5b03e_time.csv difference percentage
q1 43.79 43.44 -0.349 99.20%
q2 24.77 24.73 -0.040 99.84%
q3 36.80 37.30 0.495 101.35%
q4 41.96 41.95 -0.007 99.98%
q5 68.15 70.09 1.935 102.84%
q6 6.44 6.54 0.102 101.59%
q7 85.26 84.15 -1.102 98.71%
q8 79.92 76.28 -3.648 95.44%
q9 114.98 116.72 1.742 101.52%
q10 48.37 42.30 -6.072 87.45%
q11 19.33 19.70 0.368 101.90%
q12 26.23 25.74 -0.488 98.14%
q13 49.73 49.36 -0.370 99.26%
q14 17.95 16.10 -1.849 89.70%
q15 30.86 31.19 0.326 101.06%
q16 15.99 15.34 -0.652 95.92%
q17 122.80 121.01 -1.790 98.54%
q18 161.51 160.47 -1.038 99.36%
q19 12.24 11.95 -0.295 97.59%
q20 31.57 25.56 -6.011 80.96%
q21 241.68 233.87 -7.808 96.77%
q22 15.27 15.38 0.103 100.67%
total 1295.61 1269.17 -26.447 97.96%

FelixYBW
FelixYBW previously approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@FelixYBW FelixYBW left a comment

Choose a reason for hiding this comment

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

passed customer test

@PHILO-HE PHILO-HE added the pending velox rebase the patch will be merged after velox rebased with Meta main branch label Sep 21, 2023
@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Sep 21, 2023 •
edited
Loading

As this PR depends on a patch for velox, let's wait for velox upstream contribution and velox code rebase.

@zhouyuan zhouyuan mentioned this pull request Sep 22, 2023
Open
zhouyuan
zhouyuan reviewed Sep 22, 2023
View reviewed changes
cpp/velox/shuffle/VeloxShuffleWriter.cc Outdated
@@ -1220,6 +1223,17 @@ arrow::Status VeloxShuffleWriter::splitFixedWidthValueBuffer(const velox::RowVec
case arrow::MapType::type_id:
case arrow::ListType::type_id:
break;
case arrow::NullType::type_id: {
std::shared_ptr<arrow::Buffer> validityBuffer = nullptr;
ARROW_RETURN_NOT_OK(pool_->allocate(validityBuffer, arrow::bit_util::BytesForBits(newSize)));
Copy link
Contributor

Choose a reason for hiding this comment

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

this piece of code needs to be updated to work with main branch

@PHILO-HE PHILO-HE force-pushed the enable-null-type branch 2 times, most recently from f019ba1 to a327792 Compare October 25, 2023 05:12
@PHILO-HE PHILO-HE mentioned this pull request Nov 16, 2023
Closed
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale stale label Dec 10, 2023
@github-actions GitHub Actions
Copy link

This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.

@github-actions github-actions bot closed this Dec 21, 2023
@PHILO-HE PHILO-HE reopened this Dec 21, 2023
@github-actions github-actions bot removed the stale stale label Dec 22, 2023
@FelixYBW
Copy link
Contributor

@PHILO-HE what's the status of this PR? Is the PR submited to upstream Velox?

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Jan 2, 2024

@PHILO-HE what's the status of this PR? Is the PR submited to upstream Velox?

Hi @FelixYBW, we have submitted required PR to velox upstream, but still not merged. I will push the progress soon. Thanks!

@FelixYBW
Copy link
Contributor

equired PR to velox upstream, but still not

which PR in velox?

@PHILO-HE
Copy link
Contributor Author

equired PR to velox upstream, but still not

which PR in velox?

Hi @FelixYBW, we have 3 velox PRs for this. One is merged, two are still in progress.
facebookincubator/velox#8724
facebookincubator/velox#6996
facebookincubator/velox#8769

@WangGuangxin WangGuangxin mentioned this pull request Feb 26, 2024
Merged
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

2 similar comments
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

zhouyuan
zhouyuan approved these changes Mar 1, 2024
View reviewed changes
Copy link
Contributor

@zhouyuan zhouyuan left a comment

Choose a reason for hiding this comment

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

👍

@PHILO-HE PHILO-HE merged commit 881309c into apache:main Mar 1, 2024
19 checks passed
@FelixYBW
Copy link
Contributor

FelixYBW commented Mar 1, 2024 •
edited
Loading

@PHILO-HE 8769 isn't merged yet. is it impact anything?

@GlutenPerfBot
Copy link
Contributor

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query log/native_2996_time.csv log/native_master_02_29_2024_22d9fe3c8_time.csv difference percentage
q1 33.42 33.44 0.021 100.06%
q2 24.59 27.05 2.470 110.05%
q3 39.27 35.95 -3.315 91.56%
q4 38.03 39.22 1.190 103.13%
q5 69.87 71.30 1.428 102.04%
q6 7.03 7.43 0.400 105.69%
q7 82.40 85.83 3.436 104.17%
q8 86.20 85.20 -0.996 98.84%
q9 123.71 118.37 -5.331 95.69%
q10 43.48 43.82 0.338 100.78%
q11 20.22 20.13 -0.088 99.56%
q12 29.55 26.26 -3.291 88.86%
q13 45.32 46.41 1.086 102.40%
q14 17.70 20.53 2.833 116.01%
q15 28.40 28.82 0.415 101.46%
q16 14.20 12.90 -1.304 90.82%
q17 101.61 103.50 1.889 101.86%
q18 148.01 149.47 1.454 100.98%
q19 12.66 12.55 -0.110 99.13%
q20 26.94 28.87 1.930 107.16%
q21 222.31 227.08 4.772 102.15%
q22 13.71 13.78 0.065 100.48%
total 1228.63 1237.92 9.291 100.76%

@PHILO-HE
Copy link
Contributor Author

PHILO-HE commented Mar 1, 2024

@PHILO-HE 8769 isn't merged yet. is it impact anything?

Hi @FelixYBW, the remaining one velox pr doesn't impact on the basic functionality of NullType and our CI can pass with oap/velox main branch. So we just merged this PR.

WangGuangxin pushed a commit to WangGuangxin/gluten that referenced this pull request May 14, 2024
@PHILO-HE @WangGuangxin
[GLUTEN-2956][VL] Support Spark NullType (apache#2996)
5c85651
kecookier pushed a commit to kecookier/gluten that referenced this pull request May 20, 2024
[ONES-84683655][GLUTEN-2956][VL] Support Spark NullType (apache#2996)
d419dc4 
https://km.sankuai.com/collabpage/2195424571

------

PR Link: https://dev.sankuai.com/code/repo-detail/data/gluten/pr/37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@winningsix winningsix winningsix left review comments

@zhouyuan zhouyuan zhouyuan approved these changes

@FelixYBW FelixYBW FelixYBW left review comments

Assignees
No one assigned
Labels
pending velox rebase the patch will be merged after velox rebased with Meta main branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] Support NullType in Project
5 participants
@PHILO-HE @GlutenPerfBot @FelixYBW @zhouyuan @winningsix