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: impl batch operators to pb #986

Merged
merged 6 commits into from
Mar 17, 2022
Merged

feat: impl batch operators to pb #986

merged 6 commits into from
Mar 17, 2022

Conversation

neverchanje
Copy link
Contributor

What's changed and what's your intention?

  1. ExprImpl was able to convert to prost types.
  2. I fixed a bug where two different queries may conflict if their query IDs happen to be the same (now query_id is generated by uuid, though the possibility of id overlapping is extremely low, we should still prevent this). The TaskManager handles this case by just rejecting the conflicted query).
  3. BatchFilter, BatchLimit, BatchValues are able to convert to prost. But it seems there's a bug in Project (select 1 shows nothing). I'll fix it later.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #986 (ba2263e) into main (894ef03) will increase coverage by 0.17%.
The diff coverage is 61.73%.

@@             Coverage Diff              @@
##               main     #986      +/-   ##
============================================
+ Coverage     71.05%   71.23%   +0.17%     
  Complexity     2766     2766              
============================================
  Files           977      978       +1     
  Lines         57587    57772     +185     
  Branches       1790     1790              
============================================
+ Hits          40921    41156     +235     
+ Misses        15775    15725      -50     
  Partials        891      891              
Flag Coverage Δ
java 61.03% <ø> (ø)
rust 75.10% <61.73%> (+0.23%) ⬆️

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

Impacted Files Coverage Δ
rust/common/src/error.rs 67.88% <0.00%> (ø)
rust/frontend/src/expr/input_ref.rs 87.50% <ø> (ø)
rust/frontend/src/handler/query.rs 23.91% <0.00%> (+0.50%) ⬆️
...t/frontend/src/optimizer/plan_node/batch_filter.rs 60.86% <0.00%> (-9.14%) ⬇️
...st/frontend/src/optimizer/plan_node/batch_limit.rs 0.00% <0.00%> (ø)
.../frontend/src/optimizer/plan_node/batch_project.rs 54.54% <0.00%> (-2.60%) ⬇️
.../frontend/src/optimizer/plan_node/logical_limit.rs 0.00% <0.00%> (ø)
rust/frontend/src/test_utils.rs 86.66% <ø> (ø)
rust/frontend/src/utils/condition.rs 87.65% <ø> (ø)
rust/batch/src/task/task.rs 32.67% <33.33%> (+21.88%) ⬆️
... and 41 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@neverchanje neverchanje marked this pull request as draft March 16, 2022 11:04
Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

Rest LGTM, good work!

rust/frontend/src/expr/mod.rs Show resolved Hide resolved
rust/frontend/src/optimizer/plan_node/batch_filter.rs Outdated Show resolved Hide resolved
@neverchanje neverchanje marked this pull request as ready for review March 17, 2022 03:02
Copy link
Contributor

@skyzh skyzh left a comment

Choose a reason for hiding this comment

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

LGTM. I've defined a mock to_prost before. You may remove it and change all occurrence to to_protobuf instead.

@skyzh skyzh enabled auto-merge (squash) March 17, 2022 03:35
@skyzh
Copy link
Contributor

skyzh commented Mar 17, 2022

Seems that get_pg_fields unit test failed :(

@skyzh
Copy link
Contributor

skyzh commented Mar 17, 2022

The pg regression test failed... Is it related to this change?

I fixed a bug where two different queries may conflict if their query IDs happen to be the same (now query_id is generated by uuid, though the possibility of id overlapping is extremely low, we should still prevent this). The TaskManager handles this case by just rejecting the conflicted query).

@skyzh
Copy link
Contributor

skyzh commented Mar 17, 2022

Didn't find can not create duplicate task in logs... Maybe some other issue.

@skyzh
Copy link
Contributor

skyzh commented Mar 17, 2022

also some new errors:

io.grpc.netty.shaded.io.netty.handler.codec.http2.Http2Exception: Header size exceeded max allowed size (10240)

@skyzh
Copy link
Contributor

skyzh commented Mar 17, 2022

The CI will check for panic message, but it seems that it's a normal error 🤣

I'll change the filter for finding panic in log, so that errors can be safely ignored.

@skyzh skyzh merged commit 95b9d6f into main Mar 17, 2022
@skyzh skyzh deleted the wt-e2e branch March 17, 2022 09:05
@neverchanje
Copy link
Contributor Author

The CI will check for panic message, but it seems that it's a normal error 🤣

I'll change the filter for finding panic in log, so that errors can be safely ignored.

@skyzh Sorry for this, I really don't know why this change was made, because my local file didn't match with the version on GitHub, I totally didn't change this file.

@skyzh
Copy link
Contributor

skyzh commented Mar 18, 2022

https://singularity-data.slack.com/archives/C0358PVT2AX/p1647506847423739

If a SQL query fails, there will be ERROR and backtrace in log. The backtrace contains panic as a backtrace filename, and will lead to CI fail -- we explicitly checked for panic before.

Now we check panicked at, so we will let CI fail only when there's real panic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants