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

chore(toolchain): bump to 2022-12-12 #6159

Merged
merged 32 commits into from
Dec 12, 2022
Merged

chore(toolchain): bump to 2022-12-12 #6159

merged 32 commits into from
Dec 12, 2022

Conversation

BugenZhao
Copy link
Member

@BugenZhao BugenZhao commented Nov 1, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Bump the toolchain to 2022-12-12, which should contain the fix to rust-lang/rust#103423. We're now able to revert some workarounds, as explained in #6103.

This is mainly to fix the excessively slow compilation speed under release profile, like https://buildkite.com/risingwavelabs/docker/builds/13181.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
This reverts commit ea98e26.
Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #6159 (074a330) into main (f2975a3) will increase coverage by 0.00%.
The diff coverage is 76.19%.

@@           Coverage Diff           @@
##             main    #6159   +/-   ##
=======================================
  Coverage   73.24%   73.24%           
=======================================
  Files        1032     1031    -1     
  Lines      164648   164587   -61     
=======================================
- Hits       120598   120555   -43     
+ Misses      44050    44032   -18     
Flag Coverage Δ
rust 73.24% <76.19%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../batch/src/task/consistent_hash_shuffle_channel.rs 0.00% <ø> (ø)
src/connector/src/source/filesystem/s3/s3_dir.rs 0.00% <0.00%> (ø)
src/meta/src/lib.rs 2.40% <ø> (ø)
src/risedevtool/src/compose.rs 0.00% <0.00%> (ø)
src/risedevtool/src/compose_deploy.rs 0.00% <0.00%> (ø)
src/risedevtool/src/task/grafana_service.rs 0.00% <0.00%> (ø)
src/risedevtool/src/task/kafka_service.rs 0.00% <0.00%> (ø)
src/risedevtool/src/task/prometheus_service.rs 0.00% <0.00%> (ø)
src/risedevtool/src/task/zookeeper_service.rs 0.00% <0.00%> (ø)
...storage/src/hummock/compactor/compaction_filter.rs 96.55% <ø> (ø)
... and 41 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@BugenZhao BugenZhao changed the title chore(toolchain): bump to 2022-11-xx chore(toolchain): bump to 2022-11-21 Dec 8, 2022
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao marked this pull request as ready for review December 8, 2022 08:43
@BugenZhao
Copy link
Member Author

@huangjw806 Could you please help to test whether building the docker image under release profile finishes in a reasonable time, under this branch?

Copy link
Contributor

@huangjw806 huangjw806 left a comment

Choose a reason for hiding this comment

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

LGTM

@huangjw806
Copy link
Contributor

ou please help to test whether building the docker image under release profile finishes in a reasonable time, under this branch?

Sure.

@BugenZhao
Copy link
Member Author

BugenZhao commented Dec 8, 2022

It cannot compile with debug assertions on.

rust-lang/rust#105009

@huangjw806
Copy link
Contributor

@huangjw806 Could you please help to test whether building the docker image under release profile finishes in a reasonable time, under this branch?

I tested the docker build of this branch, it was successful and fast.
https://buildkite.com/risingwavelabs/docker/builds/13184#_

Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao
Copy link
Member Author

It seems like it's #6613 that causes the slow compilation before bumping and ICE after bumping.

cc @wenym1 Would you please help to take a look and clean some suspicious sugars or magics in that PR? 🥵

@wenym1
Copy link
Contributor

wenym1 commented Dec 9, 2022

It seems like it's #6613 that causes the slow compilation before bumping and ICE after bumping.
cc @wenym1 Would you please help to take a look and clean some suspicious sugars or magics in that PR? 🥵

In #6613, we only wrap the previous StateStoreIter with a try_stream. I am not sure whether this will cause the slow compilation. Can you provide me a way to reproduce the problem locally?

@BugenZhao
Copy link
Member Author

BugenZhao commented Dec 9, 2022

Can you provide me a way to reproduce the problem locally?

I guess compiling with release profile under Linux can reproduce this, just like what we do when building image on CI.

RUN cargo build -p risingwave_cmd -p risingwave_cmd_all --release --features "static-link static-log-level" && \

Signed-off-by: Bugen Zhao <[email protected]>
Signed-off-by: Bugen Zhao <[email protected]>
@BugenZhao BugenZhao changed the title chore(toolchain): bump to 2022-11-21 chore(toolchain): bump to 2022-12-12 Dec 12, 2022
@BugenZhao
Copy link
Member Author

rust-lang/rust#105009 has been fixed and I've successfully bumped the toolchain to 12-12. Seems that everything is working well.

cc @wenym1 There's no need to do anything with #6613 now. :)

Signed-off-by: Bugen Zhao <[email protected]>
src/storage/src/lib.rs Outdated Show resolved Hide resolved
@@ -43,7 +43,7 @@ pub enum MemTableError {
},
}

type Result<T> = std::result::Result<T, MemTableError>;
type Result<T> = std::result::Result<T, Box<MemTableError>>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This error is also large.

#[for_await]
for row_and_degree in zipped_iter {
let (row, degree): (Cow<'_, Row>, Cow<'_, Row>) = row_and_degree?;
for (row, degree) in table_iter.zip(degree_table_iter) {
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @yuhao-su Am I correct here?

@@ -5,9 +5,11 @@ statement ok
SET QUERY_MODE TO distributed;

# Create tpch tables

Copy link
Member

Choose a reason for hiding this comment

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

Revert this change? (& sqllogctest master preserves newline now 🤪

@@ -16,21 +16,21 @@ where c_state like 'A%'
group by ol_o_id, ol_w_id, ol_d_id, o_entry_d
order by revenue desc, o_entry_d;
----
2111 1 4 21 2015-11-22 00:00:00
Copy link
Member

Choose a reason for hiding this comment

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

order by revenue desc, o_entry_d seems not enough to be deterministic..

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Will keep rowsort here.

@@ -10,26 +10,25 @@ where c_id = o_c_id
and o_entry_d >= '2007-01-02 00:00:00.000000'
and o_entry_d <= ol_delivery_d
and n_nationkey = ascii(substr(c_state,1,1)) - 48
group by c_id, c_last, c_city, c_phone, n_name
order by revenue desc;
Copy link
Member

Choose a reason for hiding this comment

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

Why to remove the order by

Copy link
Member Author

Choose a reason for hiding this comment

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

I think order by makes no sense under rowsort?

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, I will revert this.

Copy link
Member

Choose a reason for hiding this comment

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

Adding order by can test sort executor produces results.. 🤪🤪🤪

Comment on lines -320 to +323
fn infer_type_name<'a, 'b>(
fn infer_type_name<'a>(
sig_map: &'a FuncSigMap,
func_type: ExprType,
inputs: &'b [Option<DataTypeName>],
inputs: &[Option<DataTypeName>],
Copy link
Member

Choose a reason for hiding this comment

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

Is this change really correct? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes

Each elided lifetime in input position becomes a distinct lifetime parameter.

@@ -28,7 +28,7 @@ RUN rustup component add rustfmt llvm-tools-preview clippy

# install build tools
RUN cargo install cargo-llvm-cov cargo-nextest cargo-udeps cargo-hakari cargo-sort cargo-make cargo-cache \
&& cargo install --git https://github.com/risinglightdb/sqllogictest-rs --bin sqllogictest \
&& cargo install --git https://github.com/risinglightdb/sqllogictest-rs --rev 65b122f --bin sqllogictest \
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to specify the revision to make the bump of sqllogictest more explicit. 😄 It's the lastest revision on main now. cc @xxchan

Copy link
Member

Choose a reason for hiding this comment

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

very good

@mergify mergify bot merged commit 885f864 into main Dec 12, 2022
@mergify mergify bot deleted the bz/bump-to-11-01 branch December 12, 2022 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants